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
