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

  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@ti.com>
Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>
---

Notes:

    changes from v6:
    * Add Neil Armstrong's R-b tag.

 drivers/gpu/drm/bridge/sii902x.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)
  

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@ti.com>
> Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

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
  

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@ti.com>
>> Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>
> 
> 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
  

Aradhya Bhatia <a-bhatia1@ti.com> writes:

Hello Aradhya,

> Hi Sam,
>

[...]

>> 
>> 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! =)
>
>

Yes, an incremental patch on top of what has already been merged in
drm-misc-next works. Thanks!