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

[Aradhya Bhatia]

Hi Sam,

On 10-Jul-23 20:38, Sam Ravnborg wrote:
> 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.

Thank you for reviewing my patch nevertheless! =)

> 
> 
>> ---
>>
>> 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.

I agree with the idea that these hooks should get streamlined.

However, this particular approach will break things when the output_fmt
is defaulted to MEDIA_BUS_FMT_FIXED. Even if we add this format as a
fall-through case along with MEDIA_BUS_FMT_RGB888_1X24, tidss driver
will too then receive MEDIA_BUS_FMT_FIXED as an expected output format
and will throw an error.

The possibility of an equivalent if-check was discussed in the previous
version[1].

> 
> 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.
> 

I am working on adding OLDI support for tidss, but if we can resolve the
above concern, and Javier agrees, I will be happy to add an incremental
fix for this! =)


Regards
Aradhya

[1]: https://patchwork.freedesktop.org/patch/536008/?series=82765&rev=6