diff mbox series

drm/bridge: tc358767: Support input format negotiation hook

Message ID 20231030192846.27934-1-a-bhatia1@ti.com
State New
Headers
Series drm/bridge: tc358767: Support input format negotiation hook |

Commit Message

Aradhya Bhatia Oct. 30, 2023, 7:28 p.m. UTC 
  With new connector model, tc358767 will not create the connector, when
DRM_BRIDGE_ATTACH_NO_CONNECTOR is set and display-controller driver will
rely on format negotiation to setup the encoder format.

Add the missing input-format negotiation hook in the
drm_bridge_funcs to complete DRM_BRIDGE_ATTACH_NO_CONNECTOR support.

Input format is selected to MEDIA_BUS_FMT_RGB888_1X24 as default, as is
the case with older model.

Reported-by: Jan Kiszka <jan.kiszka@siemens.com>
Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
---

Notes:

  * Since I do not have hardware with me, this was just build tested. I would
    appreciate it if someone could test and review it, especically somebody, who
    uses the bridge for DPI/DSI to eDP format conversion.

  * The Toshiba TC358767 bridge is not enabled in arm64 defconfig by default,
    when it should be. Hence, I sent a quick patch[0] earlier.

[0]: https://lore.kernel.org/all/20231030152834.18450-1-a-bhatia1@ti.com/

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


base-commit: c503e3eec382ac708ee7adf874add37b77c5d312

Comments

Jan Kiszka Oct. 31, 2023, 6:24 a.m. UTC | #1 
On 30.10.23 20:28, Aradhya Bhatia wrote:
> With new connector model, tc358767 will not create the connector, when
> DRM_BRIDGE_ATTACH_NO_CONNECTOR is set and display-controller driver will
> rely on format negotiation to setup the encoder format.
> 
> Add the missing input-format negotiation hook in the
> drm_bridge_funcs to complete DRM_BRIDGE_ATTACH_NO_CONNECTOR support.
> 
> Input format is selected to MEDIA_BUS_FMT_RGB888_1X24 as default, as is
> the case with older model.
> 
> Reported-by: Jan Kiszka <jan.kiszka@siemens.com>
> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
> ---
> 
> Notes:
> 
>   * Since I do not have hardware with me, this was just build tested. I would
>     appreciate it if someone could test and review it, especically somebody, who
>     uses the bridge for DPI/DSI to eDP format conversion.
> 
>   * The Toshiba TC358767 bridge is not enabled in arm64 defconfig by default,
>     when it should be. Hence, I sent a quick patch[0] earlier.
> 
> [0]: https://lore.kernel.org/all/20231030152834.18450-1-a-bhatia1@ti.com/
> 
>  drivers/gpu/drm/bridge/tc358767.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index ef2e373606ba..0affcefdeb1c 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -1751,6 +1751,30 @@ tc_dpi_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
>  	return input_fmts;
>  }
>  
> +static u32 *
> +tc_edp_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(MAX_INPUT_SEL_FORMATS, sizeof(*input_fmts),
> +			     GFP_KERNEL);
> +	if (!input_fmts)
> +		return NULL;
> +
> +	/* This is the DSI/DPI-end bus format */
> +	input_fmts[0] = MEDIA_BUS_FMT_RGB888_1X24;
> +	*num_input_fmts = 1;
> +
> +	return input_fmts;
> +}
> +
>  static const struct drm_bridge_funcs tc_dpi_bridge_funcs = {
>  	.attach = tc_dpi_bridge_attach,
>  	.mode_valid = tc_dpi_mode_valid,
> @@ -1777,6 +1801,7 @@ static const struct drm_bridge_funcs tc_edp_bridge_funcs = {
>  	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
>  	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
>  	.atomic_reset = drm_atomic_helper_bridge_reset,
> +	.atomic_get_input_bus_fmts = tc_edp_atomic_get_input_bus_fmts,
>  };
>  
>  static bool tc_readable_reg(struct device *dev, unsigned int reg)
> 
> base-commit: c503e3eec382ac708ee7adf874add37b77c5d312

Doesn't help, callback is never invoked. There must be more missing.

Regarding test setup: Maybe your colleague Preneeth can help to give you
access, he just received some devices from us. Otherwise, drop me
instrumentation patches.

Jan
Tomi Valkeinen Oct. 31, 2023, 10:53 a.m. UTC | #2 
Hi Jan,

On 31/10/2023 08:24, Jan Kiszka wrote:
> On 30.10.23 20:28, Aradhya Bhatia wrote:
>> With new connector model, tc358767 will not create the connector, when
>> DRM_BRIDGE_ATTACH_NO_CONNECTOR is set and display-controller driver will
>> rely on format negotiation to setup the encoder format.
>>
>> Add the missing input-format negotiation hook in the
>> drm_bridge_funcs to complete DRM_BRIDGE_ATTACH_NO_CONNECTOR support.
>>
>> Input format is selected to MEDIA_BUS_FMT_RGB888_1X24 as default, as is
>> the case with older model.
>>
>> Reported-by: Jan Kiszka <jan.kiszka@siemens.com>
>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>> ---
>>
>> Notes:
>>
>>    * Since I do not have hardware with me, this was just build tested. I would
>>      appreciate it if someone could test and review it, especically somebody, who
>>      uses the bridge for DPI/DSI to eDP format conversion.
>>
>>    * The Toshiba TC358767 bridge is not enabled in arm64 defconfig by default,
>>      when it should be. Hence, I sent a quick patch[0] earlier.
>>
>> [0]: https://lore.kernel.org/all/20231030152834.18450-1-a-bhatia1@ti.com/
>>
>>   drivers/gpu/drm/bridge/tc358767.c | 25 +++++++++++++++++++++++++
>>   1 file changed, 25 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
>> index ef2e373606ba..0affcefdeb1c 100644
>> --- a/drivers/gpu/drm/bridge/tc358767.c
>> +++ b/drivers/gpu/drm/bridge/tc358767.c
>> @@ -1751,6 +1751,30 @@ tc_dpi_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
>>   	return input_fmts;
>>   }
>>   
>> +static u32 *
>> +tc_edp_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(MAX_INPUT_SEL_FORMATS, sizeof(*input_fmts),
>> +			     GFP_KERNEL);
>> +	if (!input_fmts)
>> +		return NULL;
>> +
>> +	/* This is the DSI/DPI-end bus format */
>> +	input_fmts[0] = MEDIA_BUS_FMT_RGB888_1X24;
>> +	*num_input_fmts = 1;
>> +
>> +	return input_fmts;
>> +}
>> +
>>   static const struct drm_bridge_funcs tc_dpi_bridge_funcs = {
>>   	.attach = tc_dpi_bridge_attach,
>>   	.mode_valid = tc_dpi_mode_valid,
>> @@ -1777,6 +1801,7 @@ static const struct drm_bridge_funcs tc_edp_bridge_funcs = {
>>   	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
>>   	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
>>   	.atomic_reset = drm_atomic_helper_bridge_reset,
>> +	.atomic_get_input_bus_fmts = tc_edp_atomic_get_input_bus_fmts,
>>   };
>>   
>>   static bool tc_readable_reg(struct device *dev, unsigned int reg)
>>
>> base-commit: c503e3eec382ac708ee7adf874add37b77c5d312
> 
> Doesn't help, callback is never invoked. There must be more missing.
> 
> Regarding test setup: Maybe your colleague Preneeth can help to give you
> access, he just received some devices from us. Otherwise, drop me
> instrumentation patches.

Can you try with this change:

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 0affcefdeb1c..137a9f5e3cad 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -1579,6 +1579,13 @@ static struct edid *tc_get_edid(struct drm_bridge *bridge,
                                 struct drm_connector *connector)
  {
         struct tc_data *tc = bridge_to_tc(bridge);
+       int ret;
+
+       ret = tc_get_display_props(tc);
+       if (ret < 0) {
+               dev_err(tc->dev, "failed to read display props: %d\n", ret);
+               return 0;
+       }
  
         return drm_get_edid(connector, &tc->aux.ddc);
  }

  Tomi
Jan Kiszka Oct. 31, 2023, 12:18 p.m. UTC | #3 
On 31.10.23 11:53, Tomi Valkeinen wrote:
> Hi Jan,
> 
> On 31/10/2023 08:24, Jan Kiszka wrote:
>> On 30.10.23 20:28, Aradhya Bhatia wrote:
>>> With new connector model, tc358767 will not create the connector, when
>>> DRM_BRIDGE_ATTACH_NO_CONNECTOR is set and display-controller driver will
>>> rely on format negotiation to setup the encoder format.
>>>
>>> Add the missing input-format negotiation hook in the
>>> drm_bridge_funcs to complete DRM_BRIDGE_ATTACH_NO_CONNECTOR support.
>>>
>>> Input format is selected to MEDIA_BUS_FMT_RGB888_1X24 as default, as is
>>> the case with older model.
>>>
>>> Reported-by: Jan Kiszka <jan.kiszka@siemens.com>
>>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>>> ---
>>>
>>> Notes:
>>>
>>>    * Since I do not have hardware with me, this was just build
>>> tested. I would
>>>      appreciate it if someone could test and review it, especically
>>> somebody, who
>>>      uses the bridge for DPI/DSI to eDP format conversion.
>>>
>>>    * The Toshiba TC358767 bridge is not enabled in arm64 defconfig by
>>> default,
>>>      when it should be. Hence, I sent a quick patch[0] earlier.
>>>
>>> [0]:
>>> https://lore.kernel.org/all/20231030152834.18450-1-a-bhatia1@ti.com/
>>>
>>>   drivers/gpu/drm/bridge/tc358767.c | 25 +++++++++++++++++++++++++
>>>   1 file changed, 25 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/tc358767.c
>>> b/drivers/gpu/drm/bridge/tc358767.c
>>> index ef2e373606ba..0affcefdeb1c 100644
>>> --- a/drivers/gpu/drm/bridge/tc358767.c
>>> +++ b/drivers/gpu/drm/bridge/tc358767.c
>>> @@ -1751,6 +1751,30 @@ tc_dpi_atomic_get_input_bus_fmts(struct
>>> drm_bridge *bridge,
>>>       return input_fmts;
>>>   }
>>>   +static u32 *
>>> +tc_edp_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(MAX_INPUT_SEL_FORMATS, sizeof(*input_fmts),
>>> +                 GFP_KERNEL);
>>> +    if (!input_fmts)
>>> +        return NULL;
>>> +
>>> +    /* This is the DSI/DPI-end bus format */
>>> +    input_fmts[0] = MEDIA_BUS_FMT_RGB888_1X24;
>>> +    *num_input_fmts = 1;
>>> +
>>> +    return input_fmts;
>>> +}
>>> +
>>>   static const struct drm_bridge_funcs tc_dpi_bridge_funcs = {
>>>       .attach = tc_dpi_bridge_attach,
>>>       .mode_valid = tc_dpi_mode_valid,
>>> @@ -1777,6 +1801,7 @@ static const struct drm_bridge_funcs
>>> tc_edp_bridge_funcs = {
>>>       .atomic_duplicate_state =
>>> drm_atomic_helper_bridge_duplicate_state,
>>>       .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
>>>       .atomic_reset = drm_atomic_helper_bridge_reset,
>>> +    .atomic_get_input_bus_fmts = tc_edp_atomic_get_input_bus_fmts,
>>>   };
>>>     static bool tc_readable_reg(struct device *dev, unsigned int reg)
>>>
>>> base-commit: c503e3eec382ac708ee7adf874add37b77c5d312
>>
>> Doesn't help, callback is never invoked. There must be more missing.
>>
>> Regarding test setup: Maybe your colleague Preneeth can help to give you
>> access, he just received some devices from us. Otherwise, drop me
>> instrumentation patches.
> 
> Can you try with this change:
> 
> diff --git a/drivers/gpu/drm/bridge/tc358767.c
> b/drivers/gpu/drm/bridge/tc358767.c
> index 0affcefdeb1c..137a9f5e3cad 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -1579,6 +1579,13 @@ static struct edid *tc_get_edid(struct drm_bridge
> *bridge,
>                                 struct drm_connector *connector)
>  {
>         struct tc_data *tc = bridge_to_tc(bridge);
> +       int ret;
> +
> +       ret = tc_get_display_props(tc);
> +       if (ret < 0) {
> +               dev_err(tc->dev, "failed to read display props: %d\n",
> ret);
> +               return 0;
> +       }
>  
>         return drm_get_edid(connector, &tc->aux.ddc);
>  }
> 
>  Tomi
> 

Yep, that does the trick.

Thanks,
Jan

PS: Your mail client is mangling tabs - was already suspecting our
server would reformat again.
Tomi Valkeinen Oct. 31, 2023, 1:06 p.m. UTC | #4 
On 31/10/2023 14:18, Jan Kiszka wrote:
> On 31.10.23 11:53, Tomi Valkeinen wrote:
>> Hi Jan,
>>
>> On 31/10/2023 08:24, Jan Kiszka wrote:
>>> On 30.10.23 20:28, Aradhya Bhatia wrote:
>>>> With new connector model, tc358767 will not create the connector, when
>>>> DRM_BRIDGE_ATTACH_NO_CONNECTOR is set and display-controller driver will
>>>> rely on format negotiation to setup the encoder format.
>>>>
>>>> Add the missing input-format negotiation hook in the
>>>> drm_bridge_funcs to complete DRM_BRIDGE_ATTACH_NO_CONNECTOR support.
>>>>
>>>> Input format is selected to MEDIA_BUS_FMT_RGB888_1X24 as default, as is
>>>> the case with older model.
>>>>
>>>> Reported-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>>>> ---
>>>>
>>>> Notes:
>>>>
>>>>     * Since I do not have hardware with me, this was just build
>>>> tested. I would
>>>>       appreciate it if someone could test and review it, especically
>>>> somebody, who
>>>>       uses the bridge for DPI/DSI to eDP format conversion.
>>>>
>>>>     * The Toshiba TC358767 bridge is not enabled in arm64 defconfig by
>>>> default,
>>>>       when it should be. Hence, I sent a quick patch[0] earlier.
>>>>
>>>> [0]:
>>>> https://lore.kernel.org/all/20231030152834.18450-1-a-bhatia1@ti.com/
>>>>
>>>>    drivers/gpu/drm/bridge/tc358767.c | 25 +++++++++++++++++++++++++
>>>>    1 file changed, 25 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/bridge/tc358767.c
>>>> b/drivers/gpu/drm/bridge/tc358767.c
>>>> index ef2e373606ba..0affcefdeb1c 100644
>>>> --- a/drivers/gpu/drm/bridge/tc358767.c
>>>> +++ b/drivers/gpu/drm/bridge/tc358767.c
>>>> @@ -1751,6 +1751,30 @@ tc_dpi_atomic_get_input_bus_fmts(struct
>>>> drm_bridge *bridge,
>>>>        return input_fmts;
>>>>    }
>>>>    +static u32 *
>>>> +tc_edp_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(MAX_INPUT_SEL_FORMATS, sizeof(*input_fmts),
>>>> +                 GFP_KERNEL);
>>>> +    if (!input_fmts)
>>>> +        return NULL;
>>>> +
>>>> +    /* This is the DSI/DPI-end bus format */
>>>> +    input_fmts[0] = MEDIA_BUS_FMT_RGB888_1X24;
>>>> +    *num_input_fmts = 1;
>>>> +
>>>> +    return input_fmts;
>>>> +}
>>>> +
>>>>    static const struct drm_bridge_funcs tc_dpi_bridge_funcs = {
>>>>        .attach = tc_dpi_bridge_attach,
>>>>        .mode_valid = tc_dpi_mode_valid,
>>>> @@ -1777,6 +1801,7 @@ static const struct drm_bridge_funcs
>>>> tc_edp_bridge_funcs = {
>>>>        .atomic_duplicate_state =
>>>> drm_atomic_helper_bridge_duplicate_state,
>>>>        .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
>>>>        .atomic_reset = drm_atomic_helper_bridge_reset,
>>>> +    .atomic_get_input_bus_fmts = tc_edp_atomic_get_input_bus_fmts,
>>>>    };
>>>>      static bool tc_readable_reg(struct device *dev, unsigned int reg)
>>>>
>>>> base-commit: c503e3eec382ac708ee7adf874add37b77c5d312
>>>
>>> Doesn't help, callback is never invoked. There must be more missing.
>>>
>>> Regarding test setup: Maybe your colleague Preneeth can help to give you
>>> access, he just received some devices from us. Otherwise, drop me
>>> instrumentation patches.
>>
>> Can you try with this change:
>>
>> diff --git a/drivers/gpu/drm/bridge/tc358767.c
>> b/drivers/gpu/drm/bridge/tc358767.c
>> index 0affcefdeb1c..137a9f5e3cad 100644
>> --- a/drivers/gpu/drm/bridge/tc358767.c
>> +++ b/drivers/gpu/drm/bridge/tc358767.c
>> @@ -1579,6 +1579,13 @@ static struct edid *tc_get_edid(struct drm_bridge
>> *bridge,
>>                                  struct drm_connector *connector)
>>   {
>>          struct tc_data *tc = bridge_to_tc(bridge);
>> +       int ret;
>> +
>> +       ret = tc_get_display_props(tc);
>> +       if (ret < 0) {
>> +               dev_err(tc->dev, "failed to read display props: %d\n",
>> ret);
>> +               return 0;
>> +       }
>>   
>>          return drm_get_edid(connector, &tc->aux.ddc);
>>   }
>>
>>   Tomi
>>
> 
> Yep, that does the trick.

Alright, good. I'll send a proper patch.

> Thanks,
> Jan
> 
> PS: Your mail client is mangling tabs - was already suspecting our
> server would reformat again.

Nah, the above was just a quick copy paste from my terminal window, and 
when copying from the terminal it's always just spaces.

  Tomi
Sam Ravnborg Nov. 6, 2023, 12:38 p.m. UTC | #5 
Hi Aradhya,

On Tue, Oct 31, 2023 at 12:58:46AM +0530, Aradhya Bhatia wrote:
> With new connector model, tc358767 will not create the connector, when
> DRM_BRIDGE_ATTACH_NO_CONNECTOR is set and display-controller driver will
> rely on format negotiation to setup the encoder format.
> 
> Add the missing input-format negotiation hook in the
> drm_bridge_funcs to complete DRM_BRIDGE_ATTACH_NO_CONNECTOR support.
> 
> Input format is selected to MEDIA_BUS_FMT_RGB888_1X24 as default, as is
> the case with older model.
> 
> Reported-by: Jan Kiszka <jan.kiszka@siemens.com>
> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
> ---
> 
> Notes:
> 
>   * Since I do not have hardware with me, this was just build tested. I would
>     appreciate it if someone could test and review it, especically somebody, who
>     uses the bridge for DPI/DSI to eDP format conversion.
> 
>   * The Toshiba TC358767 bridge is not enabled in arm64 defconfig by default,
>     when it should be. Hence, I sent a quick patch[0] earlier.
> 
> [0]: https://lore.kernel.org/all/20231030152834.18450-1-a-bhatia1@ti.com/
> 
>  drivers/gpu/drm/bridge/tc358767.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index ef2e373606ba..0affcefdeb1c 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -1751,6 +1751,30 @@ tc_dpi_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
>  	return input_fmts;
>  }
>  
> +static u32 *
> +tc_edp_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(MAX_INPUT_SEL_FORMATS, sizeof(*input_fmts),
> +			     GFP_KERNEL);
> +	if (!input_fmts)
> +		return NULL;
> +
> +	/* This is the DSI/DPI-end bus format */
> +	input_fmts[0] = MEDIA_BUS_FMT_RGB888_1X24;
> +	*num_input_fmts = 1;
> +
> +	return input_fmts;
> +}

You could benefit from using the helper:
drm_atomic_helper_bridge_propagate_bus_fmt()

	Sam
Aradhya Bhatia Nov. 6, 2023, 7:47 p.m. UTC | #6 
Hi Sam,

Thank you for the suggestion!

On 06-Nov-23 18:08, Sam Ravnborg wrote:
> Hi Aradhya,
> 
> On Tue, Oct 31, 2023 at 12:58:46AM +0530, Aradhya Bhatia wrote:
>> With new connector model, tc358767 will not create the connector, when
>> DRM_BRIDGE_ATTACH_NO_CONNECTOR is set and display-controller driver will
>> rely on format negotiation to setup the encoder format.
>>
>> Add the missing input-format negotiation hook in the
>> drm_bridge_funcs to complete DRM_BRIDGE_ATTACH_NO_CONNECTOR support.
>>
>> Input format is selected to MEDIA_BUS_FMT_RGB888_1X24 as default, as is
>> the case with older model.
>>
>> Reported-by: Jan Kiszka <jan.kiszka@siemens.com>
>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>> ---
>>
>> Notes:
>>
>>   * Since I do not have hardware with me, this was just build tested. I would
>>     appreciate it if someone could test and review it, especically somebody, who
>>     uses the bridge for DPI/DSI to eDP format conversion.
>>
>>   * The Toshiba TC358767 bridge is not enabled in arm64 defconfig by default,
>>     when it should be. Hence, I sent a quick patch[0] earlier.
>>
>> [0]: https://lore.kernel.org/all/20231030152834.18450-1-a-bhatia1@ti.com/
>>
>>  drivers/gpu/drm/bridge/tc358767.c | 25 +++++++++++++++++++++++++
>>  1 file changed, 25 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
>> index ef2e373606ba..0affcefdeb1c 100644
>> --- a/drivers/gpu/drm/bridge/tc358767.c
>> +++ b/drivers/gpu/drm/bridge/tc358767.c
>> @@ -1751,6 +1751,30 @@ tc_dpi_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
>>  	return input_fmts;
>>  }
>>  
>> +static u32 *
>> +tc_edp_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(MAX_INPUT_SEL_FORMATS, sizeof(*input_fmts),
>> +			     GFP_KERNEL);
>> +	if (!input_fmts)
>> +		return NULL;
>> +
>> +	/* This is the DSI/DPI-end bus format */
>> +	input_fmts[0] = MEDIA_BUS_FMT_RGB888_1X24;
>> +	*num_input_fmts = 1;
>> +
>> +	return input_fmts;
>> +}
> 
> You could benefit from using the helper:
> drm_atomic_helper_bridge_propagate_bus_fmt()

You are right!

Upon taking a second look, I realize that the bridge chain works with
MEDIA_BUS_FMT_FIXED bus format, when tc358767 is being used in DPI/DSI
to eDP mode (because the panel-bridge does not have a get_output_bus_fmt
hook, and uses the same helper for its get_input_bus_fmt hook). My patch
creates a deviation from that, by forcing MEDIA_BUS_FMT_RGB888_1X24 even
when eDP is involved.

Using the helper here, will certainly address this deviation.

However, for the DPI/DSI to DP mode, MEDIA_BUS_FMT_RGB888_1X24 bus
format is required, and *just* using the helper as its get_input_bus_fmt
hook, might not be enough.

Since tc358767 is the last bridge in DPI/DSI to DP mode, the
output_fmt parameter get defaulted to MEDIA_BUS_FMT_FIXED too, as there
is no get_output_bus_fmt hook present in the driver. If we simply us
the helper here, the input_fmt will also get set to MEDIA_BUS_FMT_FIXED.
This too is an unwanted deviation.

It seems like the right way to address both the cases, would be by
adding the get_output_bus_fmt hook that sets output_fmt to
MEDIA_BUS_FMT_RGB888_1X24, as well as using the helper as the
get_input_bus_fmt hook.

If this seems good to you too, I will send a new version of Tomi's
series[0] which incorporates this patch.


Regards
Aradhya

[0]:
https://lore.kernel.org/all/20231031-tc358767-v1-0-392081ad9f4b@ideasonboard.com/
Sam Ravnborg Nov. 7, 2023, 3:41 p.m. UTC | #7 
Hi Aradhya,

On Tue, Nov 07, 2023 at 01:17:03AM +0530, Aradhya Bhatia wrote:
> Hi Sam,
> 
> Thank you for the suggestion!
> 
> On 06-Nov-23 18:08, Sam Ravnborg wrote:
> > Hi Aradhya,
> > 
> > On Tue, Oct 31, 2023 at 12:58:46AM +0530, Aradhya Bhatia wrote:
> >> With new connector model, tc358767 will not create the connector, when
> >> DRM_BRIDGE_ATTACH_NO_CONNECTOR is set and display-controller driver will
> >> rely on format negotiation to setup the encoder format.
> >>
> >> Add the missing input-format negotiation hook in the
> >> drm_bridge_funcs to complete DRM_BRIDGE_ATTACH_NO_CONNECTOR support.
> >>
> >> Input format is selected to MEDIA_BUS_FMT_RGB888_1X24 as default, as is
> >> the case with older model.
> >>
> >> Reported-by: Jan Kiszka <jan.kiszka@siemens.com>
> >> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
> >> ---
> >>
> >> Notes:
> >>
> >>   * Since I do not have hardware with me, this was just build tested. I would
> >>     appreciate it if someone could test and review it, especically somebody, who
> >>     uses the bridge for DPI/DSI to eDP format conversion.
> >>
> >>   * The Toshiba TC358767 bridge is not enabled in arm64 defconfig by default,
> >>     when it should be. Hence, I sent a quick patch[0] earlier.
> >>
> >> [0]: https://lore.kernel.org/all/20231030152834.18450-1-a-bhatia1@ti.com/
> >>
> >>  drivers/gpu/drm/bridge/tc358767.c | 25 +++++++++++++++++++++++++
> >>  1 file changed, 25 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> >> index ef2e373606ba..0affcefdeb1c 100644
> >> --- a/drivers/gpu/drm/bridge/tc358767.c
> >> +++ b/drivers/gpu/drm/bridge/tc358767.c
> >> @@ -1751,6 +1751,30 @@ tc_dpi_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
> >>  	return input_fmts;
> >>  }
> >>  
> >> +static u32 *
> >> +tc_edp_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(MAX_INPUT_SEL_FORMATS, sizeof(*input_fmts),
> >> +			     GFP_KERNEL);
> >> +	if (!input_fmts)
> >> +		return NULL;
> >> +
> >> +	/* This is the DSI/DPI-end bus format */
> >> +	input_fmts[0] = MEDIA_BUS_FMT_RGB888_1X24;
> >> +	*num_input_fmts = 1;
> >> +
> >> +	return input_fmts;
> >> +}
> > 
> > You could benefit from using the helper:
> > drm_atomic_helper_bridge_propagate_bus_fmt()
> 
> You are right!
> 
> Upon taking a second look, I realize that the bridge chain works with
> MEDIA_BUS_FMT_FIXED bus format, when tc358767 is being used in DPI/DSI
> to eDP mode (because the panel-bridge does not have a get_output_bus_fmt
> hook, and uses the same helper for its get_input_bus_fmt hook). My patch
> creates a deviation from that, by forcing MEDIA_BUS_FMT_RGB888_1X24 even
> when eDP is involved.
> 
> Using the helper here, will certainly address this deviation.
> 
> However, for the DPI/DSI to DP mode, MEDIA_BUS_FMT_RGB888_1X24 bus
> format is required, and *just* using the helper as its get_input_bus_fmt
> hook, might not be enough.
> 
> Since tc358767 is the last bridge in DPI/DSI to DP mode, the
> output_fmt parameter get defaulted to MEDIA_BUS_FMT_FIXED too, as there
> is no get_output_bus_fmt hook present in the driver. If we simply us
> the helper here, the input_fmt will also get set to MEDIA_BUS_FMT_FIXED.
> This too is an unwanted deviation.
> 
> It seems like the right way to address both the cases, would be by
> adding the get_output_bus_fmt hook that sets output_fmt to
> MEDIA_BUS_FMT_RGB888_1X24, as well as using the helper as the
> get_input_bus_fmt hook.
> 
> If this seems good to you too, I will send a new version of Tomi's
> series[0] which incorporates this patch.

I never managed to fully wrap my head around the bus fmt negotiation,
and as I am trying to recover from a flu this is not the time to try.
Your explanations sounds like you have grasped it so I suggest to move
ahead.

	Sam
Aradhya Bhatia Nov. 7, 2023, 6:31 p.m. UTC | #8 
Hi Sam,

On 07-Nov-23 21:11, Sam Ravnborg wrote:
> Hi Aradhya,
> 
> On Tue, Nov 07, 2023 at 01:17:03AM +0530, Aradhya Bhatia wrote:
>> Hi Sam,
>>
>> Thank you for the suggestion!
>>
>> On 06-Nov-23 18:08, Sam Ravnborg wrote:
>>> Hi Aradhya,
>>>
>>> On Tue, Oct 31, 2023 at 12:58:46AM +0530, Aradhya Bhatia wrote:
>>>> With new connector model, tc358767 will not create the connector, when
>>>> DRM_BRIDGE_ATTACH_NO_CONNECTOR is set and display-controller driver will
>>>> rely on format negotiation to setup the encoder format.
>>>>
>>>> Add the missing input-format negotiation hook in the
>>>> drm_bridge_funcs to complete DRM_BRIDGE_ATTACH_NO_CONNECTOR support.
>>>>
>>>> Input format is selected to MEDIA_BUS_FMT_RGB888_1X24 as default, as is
>>>> the case with older model.
>>>>
>>>> Reported-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>>>> ---
>>>>
>>>> Notes:
>>>>
>>>>   * Since I do not have hardware with me, this was just build tested. I would
>>>>     appreciate it if someone could test and review it, especically somebody, who
>>>>     uses the bridge for DPI/DSI to eDP format conversion.
>>>>
>>>>   * The Toshiba TC358767 bridge is not enabled in arm64 defconfig by default,
>>>>     when it should be. Hence, I sent a quick patch[0] earlier.
>>>>
>>>> [0]: https://lore.kernel.org/all/20231030152834.18450-1-a-bhatia1@ti.com/
>>>>
>>>>  drivers/gpu/drm/bridge/tc358767.c | 25 +++++++++++++++++++++++++
>>>>  1 file changed, 25 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
>>>> index ef2e373606ba..0affcefdeb1c 100644
>>>> --- a/drivers/gpu/drm/bridge/tc358767.c
>>>> +++ b/drivers/gpu/drm/bridge/tc358767.c
>>>> @@ -1751,6 +1751,30 @@ tc_dpi_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
>>>>  	return input_fmts;
>>>>  }
>>>>  
>>>> +static u32 *
>>>> +tc_edp_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(MAX_INPUT_SEL_FORMATS, sizeof(*input_fmts),
>>>> +			     GFP_KERNEL);
>>>> +	if (!input_fmts)
>>>> +		return NULL;
>>>> +
>>>> +	/* This is the DSI/DPI-end bus format */
>>>> +	input_fmts[0] = MEDIA_BUS_FMT_RGB888_1X24;
>>>> +	*num_input_fmts = 1;
>>>> +
>>>> +	return input_fmts;
>>>> +}
>>>
>>> You could benefit from using the helper:
>>> drm_atomic_helper_bridge_propagate_bus_fmt()
>>
>> You are right!
>>
>> Upon taking a second look, I realize that the bridge chain works with
>> MEDIA_BUS_FMT_FIXED bus format, when tc358767 is being used in DPI/DSI
>> to eDP mode (because the panel-bridge does not have a get_output_bus_fmt
>> hook, and uses the same helper for its get_input_bus_fmt hook). My patch
>> creates a deviation from that, by forcing MEDIA_BUS_FMT_RGB888_1X24 even
>> when eDP is involved.
>>
>> Using the helper here, will certainly address this deviation.
>>
>> However, for the DPI/DSI to DP mode, MEDIA_BUS_FMT_RGB888_1X24 bus
>> format is required, and *just* using the helper as its get_input_bus_fmt
>> hook, might not be enough.
>>
>> Since tc358767 is the last bridge in DPI/DSI to DP mode, the
>> output_fmt parameter get defaulted to MEDIA_BUS_FMT_FIXED too, as there
>> is no get_output_bus_fmt hook present in the driver. If we simply us
>> the helper here, the input_fmt will also get set to MEDIA_BUS_FMT_FIXED.
>> This too is an unwanted deviation.
>>
>> It seems like the right way to address both the cases, would be by
>> adding the get_output_bus_fmt hook that sets output_fmt to
>> MEDIA_BUS_FMT_RGB888_1X24, as well as using the helper as the
>> get_input_bus_fmt hook.
>>
>> If this seems good to you too, I will send a new version of Tomi's
>> series[0] which incorporates this patch.
> 
> I never managed to fully wrap my head around the bus fmt negotiation,
> and as I am trying to recover from a flu this is not the time to try.
> Your explanations sounds like you have grasped it so I suggest to move
> ahead.
> 

Sure, I will send a new version.

Hope you recover soon! =)

Regards
Aradhya
diff mbox series

Patch

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index ef2e373606ba..0affcefdeb1c 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -1751,6 +1751,30 @@  tc_dpi_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
 	return input_fmts;
 }
 
+static u32 *
+tc_edp_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(MAX_INPUT_SEL_FORMATS, sizeof(*input_fmts),
+			     GFP_KERNEL);
+	if (!input_fmts)
+		return NULL;
+
+	/* This is the DSI/DPI-end bus format */
+	input_fmts[0] = MEDIA_BUS_FMT_RGB888_1X24;
+	*num_input_fmts = 1;
+
+	return input_fmts;
+}
+
 static const struct drm_bridge_funcs tc_dpi_bridge_funcs = {
 	.attach = tc_dpi_bridge_attach,
 	.mode_valid = tc_dpi_mode_valid,
@@ -1777,6 +1801,7 @@  static const struct drm_bridge_funcs tc_edp_bridge_funcs = {
 	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
 	.atomic_reset = drm_atomic_helper_bridge_reset,
+	.atomic_get_input_bus_fmts = tc_edp_atomic_get_input_bus_fmts,
 };
 
 static bool tc_readable_reg(struct device *dev, unsigned int reg)