| Message ID | 20230606082142.23760-6-a-bhatia1@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/tidss: Use new connector model for tidss | expand |
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!
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; +} + static const struct drm_bridge_funcs sii902x_bridge_funcs = { .attach = sii902x_bridge_attach, .mode_set = sii902x_bridge_mode_set, @@ -480,6 +501,10 @@ static const struct drm_bridge_funcs sii902x_bridge_funcs = { .enable = sii902x_bridge_enable, .detect = sii902x_bridge_detect, .get_edid = sii902x_bridge_get_edid, + .atomic_reset = drm_atomic_helper_bridge_reset, + .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, + .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, + .atomic_get_input_bus_fmts = sii902x_bridge_atomic_get_input_bus_fmts, }; static int sii902x_mute(struct sii902x *sii902x, bool mute)