| Message ID | 20231030192846.27934-1-a-bhatia1@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/bridge: tc358767: Support input format negotiation hook | expand |
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
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
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.
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
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
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/
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
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 --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)
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