| Message ID | 20240530093621.1925863-8-a-bhatia1@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/bridge: cdns-dsi: Fix the color-shift issue | expand |
On Thu, May 30, 2024 at 03:06:19PM +0530, Aradhya Bhatia wrote: > Change the existing (and deprecated) bridge hooks, to the bridge > atomic APIs. > > Add drm helpers for duplicate_state, destroy_state, and bridge_reset > bridge hooks. > > Further add support for the input format negotiation hook. > > Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com> > --- > .../gpu/drm/bridge/cadence/cdns-dsi-core.c | 70 ++++++++++++++++--- > 1 file changed, 62 insertions(+), 8 deletions(-) Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> Minor nit below. > > @@ -915,13 +920,62 @@ static void cdns_dsi_bridge_pre_enable(struct drm_bridge *bridge) > cdns_dsi_hs_init(dsi); > } > > +static u32 *cdns_dsi_bridge_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) > +{ This code below looks pretty generic. Would be logical to extract it to a helper and allow it to be used by other DSI host bridges? > + struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge); > + struct cdns_dsi *dsi = input_to_dsi(input); > + struct cdns_dsi_output *output = &dsi->output; > + u32 *input_fmts; > + > + *num_input_fmts = 0; > + > + input_fmts = kzalloc(sizeof(*input_fmts), GFP_KERNEL); > + if (!input_fmts) > + return NULL; > + > + switch (output->dev->format) { > + case MIPI_DSI_FMT_RGB888: > + input_fmts[0] = MEDIA_BUS_FMT_RGB888_1X24; > + break; > + > + case MIPI_DSI_FMT_RGB666: > + input_fmts[0] = MEDIA_BUS_FMT_RGB666_1X24_CPADHI; > + break; > + > + case MIPI_DSI_FMT_RGB666_PACKED: > + input_fmts[0] = MEDIA_BUS_FMT_RGB666_1X18; > + break; > + > + case MIPI_DSI_FMT_RGB565: > + input_fmts[0] = MEDIA_BUS_FMT_RGB565_1X16; > + break; > + > + default: > + /* Unsupported DSI Format */ > + return NULL; > + } > + > + *num_input_fmts = 1; > + > + return input_fmts; > +} > +
Hi Dmitry, Thank you for reviewing the patches. On 31/05/24 04:51, Dmitry Baryshkov wrote: > On Thu, May 30, 2024 at 03:06:19PM +0530, Aradhya Bhatia wrote: >> Change the existing (and deprecated) bridge hooks, to the bridge >> atomic APIs. >> >> Add drm helpers for duplicate_state, destroy_state, and bridge_reset >> bridge hooks. >> >> Further add support for the input format negotiation hook. >> >> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com> >> --- >> .../gpu/drm/bridge/cadence/cdns-dsi-core.c | 70 ++++++++++++++++--- >> 1 file changed, 62 insertions(+), 8 deletions(-) > > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > Minor nit below. > >> >> @@ -915,13 +920,62 @@ static void cdns_dsi_bridge_pre_enable(struct drm_bridge *bridge) >> cdns_dsi_hs_init(dsi); >> } >> >> +static u32 *cdns_dsi_bridge_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) >> +{ > > This code below looks pretty generic. Would be logical to extract it to > a helper and allow it to be used by other DSI host bridges? I agree, it would indeed make sense. I just tried to make a helper function that could directly be passed to the drm_bridge_funcs list - like we do with "drm_atomic_helper_bridge_propagate_bus_fmt". This would have been ideal in my opinion. But, that doesn't seem possible today. The parameter "output_fmt" doesn't describe any of the DSI pixel formats from "enum mipi_dsi_pixel_format", which is what's required to ascertain the input bus format for dsi hosts. Neither do the drm_bridge{_state} help with that. For now, I am thinking to just add a common function that accepts the dsi bus output format and returns the corresponding input bus format. With this, every dsi host will still need to implement their own get_input_fmt hook and call that function. If you had something else in mind, do let me know! =) Regards Aradhya > >> + struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge); >> + struct cdns_dsi *dsi = input_to_dsi(input); >> + struct cdns_dsi_output *output = &dsi->output; >> + u32 *input_fmts; >> + >> + *num_input_fmts = 0; >> + >> + input_fmts = kzalloc(sizeof(*input_fmts), GFP_KERNEL); >> + if (!input_fmts) >> + return NULL; >> + >> + switch (output->dev->format) { >> + case MIPI_DSI_FMT_RGB888: >> + input_fmts[0] = MEDIA_BUS_FMT_RGB888_1X24; >> + break; >> + >> + case MIPI_DSI_FMT_RGB666: >> + input_fmts[0] = MEDIA_BUS_FMT_RGB666_1X24_CPADHI; >> + break; >> + >> + case MIPI_DSI_FMT_RGB666_PACKED: >> + input_fmts[0] = MEDIA_BUS_FMT_RGB666_1X18; >> + break; >> + >> + case MIPI_DSI_FMT_RGB565: >> + input_fmts[0] = MEDIA_BUS_FMT_RGB565_1X16; >> + break; >> + >> + default: >> + /* Unsupported DSI Format */ >> + return NULL; >> + } >> + >> + *num_input_fmts = 1; >> + >> + return input_fmts; >> +} >> + >
On Mon, Jun 10, 2024 at 06:02:41PM +0530, Aradhya Bhatia wrote: > Hi Dmitry, > > Thank you for reviewing the patches. > > On 31/05/24 04:51, Dmitry Baryshkov wrote: > > On Thu, May 30, 2024 at 03:06:19PM +0530, Aradhya Bhatia wrote: > >> Change the existing (and deprecated) bridge hooks, to the bridge > >> atomic APIs. > >> > >> Add drm helpers for duplicate_state, destroy_state, and bridge_reset > >> bridge hooks. > >> > >> Further add support for the input format negotiation hook. > >> > >> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com> > >> --- > >> .../gpu/drm/bridge/cadence/cdns-dsi-core.c | 70 ++++++++++++++++--- > >> 1 file changed, 62 insertions(+), 8 deletions(-) > > > > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > > Minor nit below. > > > >> > >> @@ -915,13 +920,62 @@ static void cdns_dsi_bridge_pre_enable(struct drm_bridge *bridge) > >> cdns_dsi_hs_init(dsi); > >> } > >> > >> +static u32 *cdns_dsi_bridge_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) > >> +{ > > > > This code below looks pretty generic. Would be logical to extract it to > > a helper and allow it to be used by other DSI host bridges? > > I agree, it would indeed make sense. > > I just tried to make a helper function that could directly be passed to > the drm_bridge_funcs list - like we do with > "drm_atomic_helper_bridge_propagate_bus_fmt". This would have been ideal > in my opinion. > > But, that doesn't seem possible today. The parameter "output_fmt" > doesn't describe any of the DSI pixel formats from "enum > mipi_dsi_pixel_format", which is what's required to ascertain the input > bus format for dsi hosts. Neither do the drm_bridge{_state} help with > that. > > For now, I am thinking to just add a common function that accepts the > dsi bus output format and returns the corresponding input bus format. > With this, every dsi host will still need to implement their own > get_input_fmt hook and call that function. > > If you had something else in mind, do let me know! =) No, the code that you have described looks pretty good. Please proceed with the implementation! > > Regards > Aradhya > > > > >> + struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge); > >> + struct cdns_dsi *dsi = input_to_dsi(input); > >> + struct cdns_dsi_output *output = &dsi->output; > >> + u32 *input_fmts; > >> + > >> + *num_input_fmts = 0; > >> + > >> + input_fmts = kzalloc(sizeof(*input_fmts), GFP_KERNEL); > >> + if (!input_fmts) > >> + return NULL; > >> + > >> + switch (output->dev->format) { > >> + case MIPI_DSI_FMT_RGB888: > >> + input_fmts[0] = MEDIA_BUS_FMT_RGB888_1X24; > >> + break; > >> + > >> + case MIPI_DSI_FMT_RGB666: > >> + input_fmts[0] = MEDIA_BUS_FMT_RGB666_1X24_CPADHI; > >> + break; > >> + > >> + case MIPI_DSI_FMT_RGB666_PACKED: > >> + input_fmts[0] = MEDIA_BUS_FMT_RGB666_1X18; > >> + break; > >> + > >> + case MIPI_DSI_FMT_RGB565: > >> + input_fmts[0] = MEDIA_BUS_FMT_RGB565_1X16; > >> + break; > >> + > >> + default: > >> + /* Unsupported DSI Format */ > >> + return NULL; > >> + } > >> + > >> + *num_input_fmts = 1; > >> + > >> + return input_fmts; > >> +} > >> + > >
diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c index 87fdd07ca0bc..6a2223ecff7d 100644 --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c @@ -13,6 +13,7 @@ #include <linux/clk.h> #include <linux/interrupt.h> #include <linux/iopoll.h> +#include <linux/media-bus-format.h> #include <linux/module.h> #include <linux/of.h> #include <linux/of_graph.h> @@ -655,7 +656,8 @@ cdns_dsi_bridge_mode_valid(struct drm_bridge *bridge, return MODE_OK; } -static void cdns_dsi_bridge_disable(struct drm_bridge *bridge) +static void cdns_dsi_bridge_atomic_disable(struct drm_bridge *bridge, + struct drm_bridge_state *old_bridge_state) { struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge); struct cdns_dsi *dsi = input_to_dsi(input); @@ -675,7 +677,8 @@ static void cdns_dsi_bridge_disable(struct drm_bridge *bridge) pm_runtime_put(dsi->base.dev); } -static void cdns_dsi_bridge_post_disable(struct drm_bridge *bridge) +static void cdns_dsi_bridge_atomic_post_disable(struct drm_bridge *bridge, + struct drm_bridge_state *old_bridge_state) { struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge); struct cdns_dsi *dsi = input_to_dsi(input); @@ -752,7 +755,8 @@ static void cdns_dsi_init_link(struct cdns_dsi *dsi) dsi->link_initialized = true; } -static void cdns_dsi_bridge_enable(struct drm_bridge *bridge) +static void cdns_dsi_bridge_atomic_enable(struct drm_bridge *bridge, + struct drm_bridge_state *old_bridge_state) { struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge); struct cdns_dsi *dsi = input_to_dsi(input); @@ -903,7 +907,8 @@ static void cdns_dsi_bridge_enable(struct drm_bridge *bridge) writel(tmp, dsi->regs + MCTL_MAIN_EN); } -static void cdns_dsi_bridge_pre_enable(struct drm_bridge *bridge) +static void cdns_dsi_bridge_atomic_pre_enable(struct drm_bridge *bridge, + struct drm_bridge_state *old_bridge_state) { struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge); struct cdns_dsi *dsi = input_to_dsi(input); @@ -915,13 +920,62 @@ static void cdns_dsi_bridge_pre_enable(struct drm_bridge *bridge) cdns_dsi_hs_init(dsi); } +static u32 *cdns_dsi_bridge_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) +{ + struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge); + struct cdns_dsi *dsi = input_to_dsi(input); + struct cdns_dsi_output *output = &dsi->output; + u32 *input_fmts; + + *num_input_fmts = 0; + + input_fmts = kzalloc(sizeof(*input_fmts), GFP_KERNEL); + if (!input_fmts) + return NULL; + + switch (output->dev->format) { + case MIPI_DSI_FMT_RGB888: + input_fmts[0] = MEDIA_BUS_FMT_RGB888_1X24; + break; + + case MIPI_DSI_FMT_RGB666: + input_fmts[0] = MEDIA_BUS_FMT_RGB666_1X24_CPADHI; + break; + + case MIPI_DSI_FMT_RGB666_PACKED: + input_fmts[0] = MEDIA_BUS_FMT_RGB666_1X18; + break; + + case MIPI_DSI_FMT_RGB565: + input_fmts[0] = MEDIA_BUS_FMT_RGB565_1X16; + break; + + default: + /* Unsupported DSI Format */ + return NULL; + } + + *num_input_fmts = 1; + + return input_fmts; +} + static const struct drm_bridge_funcs cdns_dsi_bridge_funcs = { .attach = cdns_dsi_bridge_attach, .mode_valid = cdns_dsi_bridge_mode_valid, - .disable = cdns_dsi_bridge_disable, - .pre_enable = cdns_dsi_bridge_pre_enable, - .enable = cdns_dsi_bridge_enable, - .post_disable = cdns_dsi_bridge_post_disable, + .atomic_disable = cdns_dsi_bridge_atomic_disable, + .atomic_pre_enable = cdns_dsi_bridge_atomic_pre_enable, + .atomic_enable = cdns_dsi_bridge_atomic_enable, + .atomic_post_disable = cdns_dsi_bridge_atomic_post_disable, + .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 = cdns_dsi_bridge_get_input_bus_fmts, }; static int cdns_dsi_attach(struct mipi_dsi_host *host,
Change the existing (and deprecated) bridge hooks, to the bridge atomic APIs. Add drm helpers for duplicate_state, destroy_state, and bridge_reset bridge hooks. Further add support for the input format negotiation hook. Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com> --- .../gpu/drm/bridge/cadence/cdns-dsi-core.c | 70 ++++++++++++++++--- 1 file changed, 62 insertions(+), 8 deletions(-)