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(-)
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.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Douglas Anderson dianders@chromium.org Cc: Andrzej Hajda a.hajda@samsung.com Cc: Neil Armstrong narmstrong@baylibre.com Cc: Robert Foss robert.foss@linaro.org Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@gmail.com Cc: Maxime Ripard mripard@kernel.org Cc: Thomas Zimmermann tzimmermann@suse.de --- include/drm/drm_bridge.h | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index 061d87313fac..fc00304be643 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -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
Hi,
On Sun, Feb 6, 2022 at 7:44 AM Sam Ravnborg sam@ravnborg.org wrote:
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.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Douglas Anderson dianders@chromium.org Cc: Andrzej Hajda a.hajda@samsung.com Cc: Neil Armstrong narmstrong@baylibre.com Cc: Robert Foss robert.foss@linaro.org Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@gmail.com Cc: Maxime Ripard mripard@kernel.org Cc: Thomas Zimmermann tzimmermann@suse.de
include/drm/drm_bridge.h | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index 061d87313fac..fc00304be643 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -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
Reviewed-by: Douglas Anderson dianders@chromium.org
Hi Sam,
Thank you for the patch.
On Sun, Feb 06, 2022 at 04:43:57PM +0100, Sam Ravnborg wrote:
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.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Douglas Anderson dianders@chromium.org Cc: Andrzej Hajda a.hajda@samsung.com Cc: Neil Armstrong narmstrong@baylibre.com Cc: Robert Foss robert.foss@linaro.org Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@gmail.com Cc: Maxime Ripard mripard@kernel.org Cc: Thomas Zimmermann tzimmermann@suse.de
include/drm/drm_bridge.h | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index 061d87313fac..fc00304be643 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -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.
- 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 :-)
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
#endif
Hi Laurent,
On Tue, Feb 08, 2022 at 02:30:42AM +0200, Laurent Pinchart wrote:
Hi Sam,
Thank you for the patch.
On Sun, Feb 06, 2022 at 04:43:57PM +0100, Sam Ravnborg wrote:
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.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Douglas Anderson dianders@chromium.org Cc: Andrzej Hajda a.hajda@samsung.com Cc: Neil Armstrong narmstrong@baylibre.com Cc: Robert Foss robert.foss@linaro.org Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@gmail.com Cc: Maxime Ripard mripard@kernel.org Cc: Thomas Zimmermann tzimmermann@suse.de
include/drm/drm_bridge.h | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index 061d87313fac..fc00304be643 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -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.
OK, done.
- 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 :-)
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.
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
#endif
-- Regards,
Laurent Pinchart
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 sam@ravnborg.org --- include/drm/media-bus-format.h | 53 ++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) create mode 100644 include/drm/media-bus-format.h
diff --git a/include/drm/media-bus-format.h b/include/drm/media-bus-format.h new file mode 100644 index 000000000000..d4d18f19a70f --- /dev/null +++ b/include/drm/media-bus-format.h @@ -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
Hi,
On Sun, Feb 6, 2022 at 7:44 AM Sam Ravnborg sam@ravnborg.org wrote:
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 sam@ravnborg.org
include/drm/media-bus-format.h | 53 ++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) create mode 100644 include/drm/media-bus-format.h
diff --git a/include/drm/media-bus-format.h b/include/drm/media-bus-format.h new file mode 100644 index 000000000000..d4d18f19a70f --- /dev/null +++ b/include/drm/media-bus-format.h @@ -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.
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...
- */
+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 */
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
Hi Sam,
Thank you for the patch.
On Sun, Feb 06, 2022 at 04:43:58PM +0100, Sam Ravnborg wrote:
For now the header file includes a single method to retreive the bpc
s/retreive/retrieve/
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 sam@ravnborg.org
include/drm/media-bus-format.h | 53 ++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) create mode 100644 include/drm/media-bus-format.h
diff --git a/include/drm/media-bus-format.h b/include/drm/media-bus-format.h new file mode 100644 index 000000000000..d4d18f19a70f --- /dev/null +++ b/include/drm/media-bus-format.h @@ -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 */
Wrong indentation.
- 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 ?
+#endif
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 sam@ravnborg.org --- drivers/gpu/drm/drm_atomic_helper.c | 41 +++++++++++++++++++++++++++++ include/drm/drm_atomic_helper.h | 7 +++++ 2 files changed, 48 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index a7a05e1e26bb..94f313dc196f 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -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; +} diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h index 4045e2507e11..0e81b6f637d6 100644 --- a/include/drm/drm_atomic_helper.h +++ b/include/drm/drm_atomic_helper.h @@ -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_ */
Hi,
On Sun, Feb 6, 2022 at 7:44 AM Sam Ravnborg sam@ravnborg.org wrote:
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 sam@ravnborg.org
drivers/gpu/drm/drm_atomic_helper.c | 41 +++++++++++++++++++++++++++++ include/drm/drm_atomic_helper.h | 7 +++++ 2 files changed, 48 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index a7a05e1e26bb..94f313dc196f 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -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?
- @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?
- 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'
- 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);
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).
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
Hello,
On Mon, Feb 07, 2022 at 02:32:45PM -0800, Doug Anderson wrote:
On Sun, Feb 6, 2022 at 7:44 AM Sam Ravnborg sam@ravnborg.org wrote:
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 sam@ravnborg.org
drivers/gpu/drm/drm_atomic_helper.c | 41 +++++++++++++++++++++++++++++ include/drm/drm_atomic_helper.h | 7 +++++ 2 files changed, 48 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index a7a05e1e26bb..94f313dc196f 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -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?
- @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?
- 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'
- 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);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).
I quite like that approach. We could even have a wrapper macro that adds the ARRAY_SIZE() argument automatically.
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, };
On Tue, Feb 08, 2022 at 02:44:25AM +0200, Laurent Pinchart wrote:
Hello,
On Mon, Feb 07, 2022 at 02:32:45PM -0800, Doug Anderson wrote:
On Sun, Feb 6, 2022 at 7:44 AM Sam Ravnborg sam@ravnborg.org wrote:
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 sam@ravnborg.org
drivers/gpu/drm/drm_atomic_helper.c | 41 +++++++++++++++++++++++++++++ include/drm/drm_atomic_helper.h | 7 +++++ 2 files changed, 48 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index a7a05e1e26bb..94f313dc196f 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -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?
- @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?
- 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'
- 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);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).
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
Move away from the deprecated enable/diable operations in drm_bridge_funcs and enable atomic use.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Douglas Anderson dianders@chromium.org Cc: Andrzej Hajda a.hajda@samsung.com Cc: Neil Armstrong narmstrong@baylibre.com Cc: Robert Foss robert.foss@linaro.org Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@gmail.com --- drivers/gpu/drm/bridge/ti-sn65dsi86.c | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index ba136a188be7..d681ab68205c 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -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,
Hi,
On Sun, Feb 6, 2022 at 7:44 AM Sam Ravnborg sam@ravnborg.org wrote:
Move away from the deprecated enable/diable operations in drm_bridge_funcs and enable atomic use.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Douglas Anderson dianders@chromium.org Cc: Andrzej Hajda a.hajda@samsung.com Cc: Neil Armstrong narmstrong@baylibre.com Cc: Robert Foss robert.foss@linaro.org Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@gmail.com
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index ba136a188be7..d681ab68205c 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -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,
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...
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
Hi Sam,
Thank you for the patch.
On Sun, Feb 06, 2022 at 04:44:00PM +0100, Sam Ravnborg wrote:
Move away from the deprecated enable/diable operations in
s/diable/disable/
drm_bridge_funcs and enable atomic use.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Douglas Anderson dianders@chromium.org Cc: Andrzej Hajda a.hajda@samsung.com Cc: Neil Armstrong narmstrong@baylibre.com Cc: Robert Foss robert.foss@linaro.org Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@gmail.com
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index ba136a188be7..d681ab68205c 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -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,
With the compilation fix,
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
};
static void ti_sn_bridge_parse_lanes(struct ti_sn65dsi86 *pdata,
To 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.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Douglas Anderson dianders@chromium.org Cc: Andrzej Hajda a.hajda@samsung.com Cc: Neil Armstrong narmstrong@baylibre.com Cc: Robert Foss robert.foss@linaro.org Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@gmail.com --- drivers/gpu/drm/bridge/ti-sn65dsi86.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index d681ab68205c..dc6ec40bc1ef 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -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)))
Hi,
On Sun, Feb 6, 2022 at 7:44 AM Sam Ravnborg sam@ravnborg.org wrote:
To 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.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Douglas Anderson dianders@chromium.org Cc: Andrzej Hajda a.hajda@samsung.com Cc: Neil Armstrong narmstrong@baylibre.com Cc: Robert Foss robert.foss@linaro.org Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@gmail.com
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index d681ab68205c..dc6ec40bc1ef 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -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;
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
On Mon, Feb 07, 2022 at 02:34:10PM -0800, Doug Anderson wrote:
Hi,
On Sun, Feb 6, 2022 at 7:44 AM Sam Ravnborg sam@ravnborg.org wrote:
To 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.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Douglas Anderson dianders@chromium.org Cc: Andrzej Hajda a.hajda@samsung.com Cc: Neil Armstrong narmstrong@baylibre.com Cc: Robert Foss robert.foss@linaro.org Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@gmail.com
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index d681ab68205c..dc6ec40bc1ef 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -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;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?
I skipped a patch to find the connector - will try to give that a spin again. Thanks for the testing and the feedback!
Sam
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
From: Rob Clark robdclark@chromium.org
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)
Signed-off-by: Rob Clark robdclark@chromium.org Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Rob Clark robdclark@chromium.org Cc: Douglas Anderson dianders@chromium.org Cc: Andrzej Hajda a.hajda@samsung.com Cc: Neil Armstrong narmstrong@baylibre.com Cc: Robert Foss robert.foss@linaro.org Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@gmail.com --- drivers/gpu/drm/bridge/ti-sn65dsi86.c | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index dc6ec40bc1ef..a9041dfd2ae5 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -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;
Hi,
On Sun, Feb 6, 2022 at 7:44 AM Sam Ravnborg sam@ravnborg.org wrote:
From: Rob Clark robdclark@chromium.org
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)
Signed-off-by: Rob Clark robdclark@chromium.org Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Rob Clark robdclark@chromium.org Cc: Douglas Anderson dianders@chromium.org Cc: Andrzej Hajda a.hajda@samsung.com Cc: Neil Armstrong narmstrong@baylibre.com Cc: Robert Foss robert.foss@linaro.org Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@gmail.com
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.
Reviewed-by: Douglas Anderson dianders@chromium.org
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.I3ab26b7f197cc56c874246a43e579...
Hello,
On Mon, Feb 07, 2022 at 02:34:34PM -0800, Doug Anderson wrote:
On Sun, Feb 6, 2022 at 7:44 AM Sam Ravnborg sam@ravnborg.org wrote:
From: Rob Clark robdclark@chromium.org
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)
Signed-off-by: Rob Clark robdclark@chromium.org Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Rob Clark robdclark@chromium.org Cc: Douglas Anderson dianders@chromium.org Cc: Andrzej Hajda a.hajda@samsung.com Cc: Neil Armstrong narmstrong@baylibre.com Cc: Robert Foss robert.foss@linaro.org Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@gmail.com
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.
Reviewed-by: Douglas Anderson dianders@chromium.org
Likewise,
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
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?
I don't really mind either way, but I of course would favour removal of connector support as soon as practical :-)
[1] https://lore.kernel.org/r/20220204161245.v2.1.I3ab26b7f197cc56c874246a43e579...
Hi Douglas,
On Mon, Feb 07, 2022 at 02:34:34PM -0800, Doug Anderson wrote:
Hi,
On Sun, Feb 6, 2022 at 7:44 AM Sam Ravnborg sam@ravnborg.org wrote:
From: Rob Clark robdclark@chromium.org
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)
Signed-off-by: Rob Clark robdclark@chromium.org Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Rob Clark robdclark@chromium.org Cc: Douglas Anderson dianders@chromium.org Cc: Andrzej Hajda a.hajda@samsung.com Cc: Neil Armstrong narmstrong@baylibre.com Cc: Robert Foss robert.foss@linaro.org Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@gmail.com
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.
Reviewed-by: Douglas Anderson dianders@chromium.org
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?
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
[1] https://lore.kernel.org/r/20220204161245.v2.1.I3ab26b7f197cc56c874246a43e579...
Hi Sam,
Quoting Sam Ravnborg (2022-02-06 15:44:02)
From: Rob Clark robdclark@chromium.org
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)
Signed-off-by: Rob Clark robdclark@chromium.org Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Rob Clark robdclark@chromium.org Cc: Douglas Anderson dianders@chromium.org Cc: Andrzej Hajda a.hajda@samsung.com Cc: Neil Armstrong narmstrong@baylibre.com Cc: Robert Foss robert.foss@linaro.org Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@gmail.com
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index dc6ec40bc1ef..a9041dfd2ae5 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -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))
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
drm_connector_cleanup(&pdata->connector);err_conn_init: drm_dp_aux_unregister(&pdata->aux); return ret; -- 2.32.0
The 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
Signed-off-by: Sam Ravnborg sam@ravnborg.org Reviewed-by: Maxime Ripard maxime@cerno.tech Cc: Jitao Shi jitao.shi@mediatek.com Cc: Enric Balletbo i Serra enric.balletbo@collabora.com Cc: Philip Chen philipchen@chromium.org Cc: Andrzej Hajda a.hajda@samsung.com Cc: Neil Armstrong narmstrong@baylibre.com Cc: Robert Foss robert.foss@linaro.org Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@gmail.com --- drivers/gpu/drm/bridge/parade-ps8640.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c index 3f17337ee389..d63c762fcd34 100644 --- a/drivers/gpu/drm/bridge/parade-ps8640.c +++ b/drivers/gpu/drm/bridge/parade-ps8640.c @@ -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)
Use the helper drm_atomic_helper_bridge_dsi_input_bus_fmt to announce the DSI media bus format.
Signed-off-by: Sam Ravnborg sam@ravnborg.org --- drivers/gpu/drm/bridge/parade-ps8640.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c index d63c762fcd34..23af23522eee 100644 --- a/drivers/gpu/drm/bridge/parade-ps8640.c +++ b/drivers/gpu/drm/bridge/parade-ps8640.c @@ -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, };
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 sam@ravnborg.org Reviewed-by: Maxime Ripard maxime@cerno.tech Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard mripard@kernel.org Cc: Thomas Zimmermann tzimmermann@suse.de Cc: Andrzej Hajda a.hajda@samsung.com Cc: Neil Armstrong narmstrong@baylibre.com Cc: Robert Foss robert.foss@linaro.org Cc: Daniel Vetter daniel@ffwll.ch --- drivers/gpu/drm/drm_bridge.c | 110 ----------------------------------- include/drm/drm_bridge.h | 28 --------- 2 files changed, 138 deletions(-)
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index c96847fc0ebc..7a57d6816105 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -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 diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index fc00304be643..ed2e4a8fe510 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -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,
Hi Sam,
Thank you for the patch.
On Sun, Feb 06, 2022 at 04:44:05PM +0100, Sam Ravnborg wrote:
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 sam@ravnborg.org Reviewed-by: Maxime Ripard maxime@cerno.tech
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard mripard@kernel.org Cc: Thomas Zimmermann tzimmermann@suse.de Cc: Andrzej Hajda a.hajda@samsung.com Cc: Neil Armstrong narmstrong@baylibre.com Cc: Robert Foss robert.foss@linaro.org Cc: Daniel Vetter daniel@ffwll.ch
drivers/gpu/drm/drm_bridge.c | 110 ----------------------------------- include/drm/drm_bridge.h | 28 --------- 2 files changed, 138 deletions(-)
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index c96847fc0ebc..7a57d6816105 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -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
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index fc00304be643..ed2e4a8fe510 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -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.**/ void (*atomic_pre_enable)(struct drm_bridge *bridge,
- The @atomic_pre_enable callback is optional.
@@ -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.**/ void (*atomic_enable)(struct drm_bridge *bridge,
- The @atomic_enable callback is optional.
@@ -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.**/ void (*atomic_disable)(struct drm_bridge *bridge,
- The @atomic_disable callback is optional.
@@ -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.**/ void (*atomic_post_disable)(struct drm_bridge *bridge,
- The @atomic_post_disable callback is optional.
@@ -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,
Hi Sam,
Quoting Sam Ravnborg (2022-02-06 19:09:11)
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,
On Wed, Jun 22, 2022 at 11:07:26AM +0100, Kieran Bingham wrote:
Hi Sam,
Quoting Sam Ravnborg (2022-02-06 19:09:11)
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.
SamDo 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
dri-devel@lists.freedesktop.org
-
Doug Anderson -
Kieran Bingham -
Laurent Pinchart -
Sam Ravnborg