Re: [PATCH 1/7] drm/bridge: display-connector: implement bus fmts callbacks

[Sam Ravnborg <sam@ravnborg.org>]

Hi Neil,

code looks fine. A few improvement proposals to the comments.
With the include order fixed and the comments considered:
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>

	Sam

On Thu, Oct 14, 2021 at 05:26:00PM +0200, Neil Armstrong wrote:
> Since this bridge is tied to the connector, it acts like a passthrough,
> so concerning the output & input bus formats, either pass the bus formats from the
> previous bridge or return fallback data like done in the bridge function:
> drm_atomic_bridge_chain_select_bus_fmts() & select_bus_fmt_recursive.
> 
> This permits avoiding skipping the negociation if the remaining bridge chain has
> all the bits in place.
> 
> Without this bus fmt negociation breaks on drm/meson HDMI pipeline when attaching
> dw-hdmi with DRM_BRIDGE_ATTACH_NO_CONNECTOR, because the last bridge of the
> display-connector doesn't implement buf fmt callbacks and MEDIA_BUS_FMT_FIXED
> is used leading to select an unsupported default bus format from dw-hdmi.
> 
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  drivers/gpu/drm/bridge/display-connector.c | 88 ++++++++++++++++++++++
>  1 file changed, 88 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/display-connector.c b/drivers/gpu/drm/bridge/display-connector.c
> index 05eb759da6fc..9697ac173157 100644
> --- a/drivers/gpu/drm/bridge/display-connector.c
> +++ b/drivers/gpu/drm/bridge/display-connector.c
> @@ -14,6 +14,7 @@
>  #include <linux/regulator/consumer.h>
>  
>  #include <drm/drm_bridge.h>
> +#include <drm/drm_atomic_helper.h>
>  #include <drm/drm_edid.h>
Alphabetic order.

>  
>  struct display_connector {
> @@ -87,10 +88,97 @@ static struct edid *display_connector_get_edid(struct drm_bridge *bridge,
>  	return drm_get_edid(connector, conn->bridge.ddc);
>  }
>  
> +/*
> + * Since this bridge is tied to the connector, it acts like a passthrough,
> + * so concerning the output bus formats, either pass the bus formats from the
> + * previous bridge or return fallback data like done in the bridge function:
> + * drm_atomic_bridge_chain_select_bus_fmts().
> + * This permits avoiding skipping the negociation if the bridge chain has all
> + * bits in place.
negociation if => negotiation of

Consider the following wording:
This supports negotiation if the bridge chain has all bits in place.

> + */
> +static u32 *display_connector_get_output_bus_fmts(struct drm_bridge *bridge,
> +					struct drm_bridge_state *bridge_state,
> +					struct drm_crtc_state *crtc_state,
> +					struct drm_connector_state *conn_state,
> +					unsigned int *num_output_fmts)
> +{
> +	struct drm_bridge *prev_bridge = drm_bridge_get_prev_bridge(bridge);
> +	struct drm_bridge_state *prev_bridge_state;
> +
> +	if (!prev_bridge || !prev_bridge->funcs->atomic_get_output_bus_fmts) {
> +		struct drm_connector *conn = conn_state->connector;
> +		u32 *out_bus_fmts;
> +
> +		*num_output_fmts = 1;
> +		out_bus_fmts = kmalloc(sizeof(*out_bus_fmts), GFP_KERNEL);
> +		if (!out_bus_fmts)
> +			return NULL;
> +
> +		if (conn->display_info.num_bus_formats &&
> +		    conn->display_info.bus_formats)
> +			out_bus_fmts[0] = conn->display_info.bus_formats[0];
> +		else
> +			out_bus_fmts[0] = MEDIA_BUS_FMT_FIXED;
> +
> +		return out_bus_fmts;
> +	}
> +
> +	prev_bridge_state = drm_atomic_get_new_bridge_state(crtc_state->state,
> +							    prev_bridge);
> +
> +	return prev_bridge->funcs->atomic_get_output_bus_fmts(prev_bridge, prev_bridge_state,
> +							      crtc_state, conn_state,
> +							      num_output_fmts);
> +}
> +
> +/*
> + * Since this bridge is tied to the connector, it acts like a passthrough,
> + * so concerning the input bus formats, either pass the bus formats from the
> + * previous bridge or return fallback data like done in the bridge function:
> + * select_bus_fmt_recursive() when atomic_get_input_bus_fmts is not supported.
Maybe use this this:
from the previous bridge or MEDIA_BUS_FMT_FIXED (like select_bus_fmt_recursive())


> + * This permits avoiding skipping the negociation if the bridge chain has all
> + * bits in place.
Like above
> + */
> +static u32 *display_connector_get_input_bus_fmts(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)
> +{
> +	struct drm_bridge *prev_bridge = drm_bridge_get_prev_bridge(bridge);
> +	struct drm_bridge_state *prev_bridge_state;
> +
> +	if (!prev_bridge || !prev_bridge->funcs->atomic_get_input_bus_fmts) {
> +		u32 *in_bus_fmts;
> +
> +		*num_input_fmts = 1;
> +		in_bus_fmts = kmalloc(sizeof(*in_bus_fmts), GFP_KERNEL);
> +		if (!in_bus_fmts)
> +			return NULL;
> +
> +		in_bus_fmts[0] = MEDIA_BUS_FMT_FIXED;
> +
> +		return in_bus_fmts;
> +	}
> +
> +	prev_bridge_state = drm_atomic_get_new_bridge_state(crtc_state->state,
> +							    prev_bridge);
> +
> +	return prev_bridge->funcs->atomic_get_input_bus_fmts(prev_bridge, prev_bridge_state,
> +							     crtc_state, conn_state, output_fmt,
> +							     num_input_fmts);
> +}
> +
>  static const struct drm_bridge_funcs display_connector_bridge_funcs = {
>  	.attach = display_connector_attach,
>  	.detect = display_connector_detect,
>  	.get_edid = display_connector_get_edid,
> +	.atomic_get_output_bus_fmts = display_connector_get_output_bus_fmts,
> +	.atomic_get_input_bus_fmts = display_connector_get_input_bus_fmts,
> +	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
> +	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
> +	.atomic_reset = drm_atomic_helper_bridge_reset,
>  };
>  
>  static irqreturn_t display_connector_hpd_irq(int irq, void *arg)
> -- 
> 2.25.1

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic