Re: [PATCH v7 5/8] drm/bridge: sii902x: Support format negotiation hooks

[Sam Ravnborg]

Hi Aradhya,

On Tue, Jun 06, 2023 at 01:51:39PM +0530, Aradhya Bhatia wrote:
> With new connector model, sii902x will not create the connector, when
> DRM_BRIDGE_ATTACH_NO_CONNECTOR is set 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.
> 
> Input format is selected to MEDIA_BUS_FMT_RGB888_1X24 as default, as is
> the case with older model.
> 
> Signed-off-by: Aradhya Bhatia <a-bhatia1@xxxxxx>
> Reviewed-by: Neil Armstrong <neil.armstrong@xxxxxxxxxx>

As noted by Javier, this patch-set was forgotten, so sorry for not
providing timely feedback.


> ---
> 
> Notes:
> 
>     changes from v6:
>     * Add Neil Armstrong's R-b tag.
> 
>  drivers/gpu/drm/bridge/sii902x.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
> index ef66461e7f7c..70aeb04b7f77 100644
> --- a/drivers/gpu/drm/bridge/sii902x.c
> +++ b/drivers/gpu/drm/bridge/sii902x.c
> @@ -473,6 +473,27 @@ static struct edid *sii902x_bridge_get_edid(struct drm_bridge *bridge,
>  	return sii902x_get_edid(sii902x, connector);
>  }
>  
> +static u32 *sii902x_bridge_atomic_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)
> +{
> +	u32 *input_fmts;
> +
> +	*num_input_fmts = 0;
> +
> +	input_fmts = kcalloc(1, sizeof(*input_fmts), GFP_KERNEL);
> +	if (!input_fmts)
> +		return NULL;
> +
> +	input_fmts[0] = MEDIA_BUS_FMT_RGB888_1X24;
> +	*num_input_fmts = 1;
> +
> +	return input_fmts;
> +}

An alternative implementation of the above is:
{
        switch (output_fmt) {
        case MEDIA_BUS_FMT_RGB888_1X24:
                break;

        default:
        /* Fail for any other formats */
               *num_input_fmts = 0;
                return NULL;
        }

       return drm_atomic_helper_bridge_propagate_bus_fmt(bridge, bridge_state,
                                                         crtc_state, conn_state,
                                                         output_fmt,
                                                         num_input_fmts);
}

If you agree and have the time to do it it would be nice to use this
simpler variant.
Mostly so we avoid more open coded variants like you already did, and
which we have plenty of already.

It would be even better to walk through other implementations of
get_input_bus_fmts and update them accordingly.

Again, sorry for being late here. Feel free to ignore if you already
moved on with something else.

	Sam