Re: [PATCH 4/5] drm/bridge: tfp410: Support format negotiation

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Nikhil,

Thank you for the patch.

On Fri, Oct 16, 2020 at 04:09:16PM +0530, Nikhil Devshatwar wrote:
> With new connector model, tfp410 will not create the connector and
> SoC driver will rely on format negotiation to setup the encoder format.
> 
> Support format negotiations hooks in the drm_bridge_funcs.
> Use helper functions for state management.
> Support one of the two RGB formats as selected from DT bindings.
> 
> Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
> ---
>  drivers/gpu/drm/bridge/ti-tfp410.c | 32 ++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c b/drivers/gpu/drm/bridge/ti-tfp410.c
> index ba3fa2a9b8a4..b65e48e080c7 100644
> --- a/drivers/gpu/drm/bridge/ti-tfp410.c
> +++ b/drivers/gpu/drm/bridge/ti-tfp410.c
> @@ -204,12 +204,44 @@ static enum drm_mode_status tfp410_mode_valid(struct drm_bridge *bridge,
>  	return MODE_OK;
>  }
>  
> +static u32 *tfp410_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 tfp410 *dvi = drm_bridge_to_tfp410(bridge);
> +	u32 *input_fmts;
> +
> +	*num_input_fmts = 0;
> +
> +	/*
> +	 * This bridge does not support media_bus_format conversion
> +	 * Propagate only if supported
> +	 */
> +	if (output_fmt != dvi->bus_format && output_fmt != MEDIA_BUS_FMT_FIXED)
> +		return NULL;

On the output side, DVI transmits RGB24 data over three TMDS links (plus
one link for the clock). There's no media bus format that specifically
describe this, but among the possible values for dvi->bus_format
(MEDIA_BUS_FMT_RGB888_2X12_LE and MEDIA_BUS_FMT_RGB888_1X24),
MEDIA_BUS_FMT_RGB888_2X12_LE doesn't make any sense to describe the
output. We probably would need to introduce a specific media bus format
if we wanted to describe this accurately
(MEDIA_BUS_FMT_RGB888_DVI_SINGLE ?), which seems a bit overkill to
support single link DVI. If we take dual link DVI into account, more bit
depths are supported, as well as faster rates by transmitting to RGB888
pixels per clock, so more codes would be needed.

With support for single-link DVI only, we could decide, subsystem-wide,
to always use MEDIA_BUS_FMT_FIXED for DVI. We could also decide to
additional accept MEDIA_BUS_FMT_RGB888_1X24 to describe single-link DVI,
as a convention.

If the goal of the above check is to make the format negotiation fail
when the desired output format can't be supported by the TFP410, then I
would use

	if (output_fmt != dvi->MEDIA_BUS_FMT_RGB888_1X24 &&
	    output_fmt != MEDIA_BUS_FMT_FIXED)
		return NULL;

or simply

	if (output_fmt != MEDIA_BUS_FMT_FIXED)
		return NULL;

depending on what convention we enforce in the subsystem for DVI media
bus formats. I however wonder if this is needed at all, are there cases
where the output could support multiple bus formats and the tfp410
driver would need to make sure that only the 24-bit single link DVI gets
selected ? I suppose there are if we consider dual link DVI, but the DVI
connector driver (drivers/gpu/drm/bridge/display-connector.c) doesn't
report bus formats anyway.

> +
> +	input_fmts = kzalloc(sizeof(*input_fmts), GFP_KERNEL);
> +	if (!input_fmts)
> +		return NULL;
> +
> +	*num_input_fmts = 1;
> +	input_fmts[0] = dvi->bus_format;
> +	return input_fmts;
> +}
> +
>  static const struct drm_bridge_funcs tfp410_bridge_funcs = {
>  	.attach		= tfp410_attach,
>  	.detach		= tfp410_detach,
>  	.enable		= tfp410_enable,
>  	.disable	= tfp410_disable,
>  	.mode_valid	= tfp410_mode_valid,
> +	.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,
> +	.atomic_get_input_bus_fmts = tfp410_get_input_bus_fmts,
>  };
>  
>  static const struct drm_bridge_timings tfp410_default_timings = {

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel