| Message ID | 20230125113529.13952-2-a-bhatia1@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add DSS support for AM625 SoC | expand |
On 25/01/2023 13:35, Aradhya Bhatia wrote: > Make DSS Video Ports agnostic of output bus types. > > DSS controllers have had a 1-to-1 coupling between its VPs and its > output ports. This no longer stands true for the new AM625 DSS. This > coupling, hence, has been removed by renaming the 'vp_bus_type' to > 'output_port_bus_type' because the VPs are essentially agnostic of the > bus type and it is the output ports which have a bus type. > > The AM625 DSS has 2 VPs but requires 3 output ports to support its > Dual-Link OLDI video output coming from a single VP. Not a biggie, but this sentence is a bit odd here at the end. Shouldn't it be after the "...stands true for the new AM625 DSS."? > Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com> > --- > drivers/gpu/drm/tidss/tidss_dispc.c | 47 +++++++++++++++++------------ > drivers/gpu/drm/tidss/tidss_dispc.h | 21 +++++++------ > drivers/gpu/drm/tidss/tidss_drv.h | 5 +-- > drivers/gpu/drm/tidss/tidss_irq.h | 2 +- > drivers/gpu/drm/tidss/tidss_kms.c | 12 ++++---- > 5 files changed, 48 insertions(+), 39 deletions(-) > > diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c > index 165365b515e1..c1c4faccbddc 100644 > --- a/drivers/gpu/drm/tidss/tidss_dispc.c > +++ b/drivers/gpu/drm/tidss/tidss_dispc.c > @@ -61,7 +61,7 @@ const struct dispc_features dispc_k2g_feats = { > .min_pclk_khz = 4375, > > .max_pclk_khz = { > - [DISPC_VP_DPI] = 150000, > + [DISPC_PORT_DPI] = 150000, > }, > > /* > @@ -96,7 +96,6 @@ const struct dispc_features dispc_k2g_feats = { > .vp_name = { "vp1" }, > .ovr_name = { "ovr1" }, > .vpclk_name = { "vp1" }, > - .vp_bus_type = { DISPC_VP_DPI }, > > .vp_feat = { .color = { > .has_ctm = true, > @@ -109,6 +108,9 @@ const struct dispc_features dispc_k2g_feats = { > .vid_name = { "vid1" }, > .vid_lite = { false }, > .vid_order = { 0 }, > + > + .num_output_ports = 1, > + .output_port_bus_type = { DISPC_PORT_DPI }, > }; Just thinking out loud, as these will get more complex in the future, maybe we should finally group them with struct. E.g. we could define struct array for vps, like (just hacky example): struct { const char *name; const char *clkname; struct tidss_vp_feat feat; } vps[TIDSS_MAX_PORTS]; and then use them as: .vps = { { .name = "kala", .clkname = "kissa", .feat.color.has_ctm = true, }, { .name = "kala2", .clkname = "kissa2", .feat.color.has_ctm = false, }, }, Perhaps something to try in the future. > static const u16 tidss_am65x_common_regs[DISPC_COMMON_REG_TABLE_LEN] = { > @@ -140,8 +142,8 @@ static const u16 tidss_am65x_common_regs[DISPC_COMMON_REG_TABLE_LEN] = { > > const struct dispc_features dispc_am65x_feats = { > .max_pclk_khz = { > - [DISPC_VP_DPI] = 165000, > - [DISPC_VP_OLDI] = 165000, > + [DISPC_PORT_DPI] = 165000, > + [DISPC_PORT_OLDI] = 165000, > }, > > .scaling = { > @@ -171,7 +173,6 @@ const struct dispc_features dispc_am65x_feats = { > .vp_name = { "vp1", "vp2" }, > .ovr_name = { "ovr1", "ovr2" }, > .vpclk_name = { "vp1", "vp2" }, > - .vp_bus_type = { DISPC_VP_OLDI, DISPC_VP_DPI }, > > .vp_feat = { .color = { > .has_ctm = true, > @@ -185,6 +186,9 @@ const struct dispc_features dispc_am65x_feats = { > .vid_name = { "vid", "vidl1" }, > .vid_lite = { false, true, }, > .vid_order = { 1, 0 }, > + > + .num_output_ports = 2, > + .output_port_bus_type = { DISPC_PORT_OLDI, DISPC_PORT_DPI }, > }; > > static const u16 tidss_j721e_common_regs[DISPC_COMMON_REG_TABLE_LEN] = { > @@ -229,8 +233,8 @@ static const u16 tidss_j721e_common_regs[DISPC_COMMON_REG_TABLE_LEN] = { > > const struct dispc_features dispc_j721e_feats = { > .max_pclk_khz = { > - [DISPC_VP_DPI] = 170000, > - [DISPC_VP_INTERNAL] = 600000, > + [DISPC_PORT_DPI] = 170000, > + [DISPC_PORT_INTERNAL] = 600000, > }, > > .scaling = { > @@ -260,9 +264,7 @@ const struct dispc_features dispc_j721e_feats = { > .vp_name = { "vp1", "vp2", "vp3", "vp4" }, > .ovr_name = { "ovr1", "ovr2", "ovr3", "ovr4" }, > .vpclk_name = { "vp1", "vp2", "vp3", "vp4" }, > - /* Currently hard coded VP routing (see dispc_initial_config()) */ > - .vp_bus_type = { DISPC_VP_INTERNAL, DISPC_VP_DPI, > - DISPC_VP_INTERNAL, DISPC_VP_DPI, }, > + I think this line feed is extra. > .vp_feat = { .color = { > .has_ctm = true, > .gamma_size = 1024, > @@ -273,6 +275,11 @@ const struct dispc_features dispc_j721e_feats = { > .vid_name = { "vid1", "vidl1", "vid2", "vidl2" }, > .vid_lite = { 0, 1, 0, 1, }, > .vid_order = { 1, 3, 0, 2 }, > + > + .num_output_ports = 4, > + /* Currently hard coded VP routing (see dispc_initial_config()) */ > + .output_port_bus_type = { DISPC_PORT_INTERNAL, DISPC_PORT_DPI, > + DISPC_PORT_INTERNAL, DISPC_PORT_DPI, }, Indent doesn't look right (but it might be just because this is a diff). > }; > > static const u16 *dispc_common_regmap; > @@ -287,12 +294,12 @@ struct dispc_device { > > void __iomem *base_common; > void __iomem *base_vid[TIDSS_MAX_PLANES]; > - void __iomem *base_ovr[TIDSS_MAX_PORTS]; > - void __iomem *base_vp[TIDSS_MAX_PORTS]; > + void __iomem *base_ovr[TIDSS_MAX_VPS]; > + void __iomem *base_vp[TIDSS_MAX_VPS]; > > struct regmap *oldi_io_ctrl; > > - struct clk *vp_clk[TIDSS_MAX_PORTS]; > + struct clk *vp_clk[TIDSS_MAX_VPS]; > > const struct dispc_features *feat; > > @@ -300,7 +307,7 @@ struct dispc_device { > > bool is_enabled; > > - struct dss_vp_data vp_data[TIDSS_MAX_PORTS]; > + struct dss_vp_data vp_data[TIDSS_MAX_VPS]; > > u32 *fourccs; > u32 num_fourccs; > @@ -851,7 +858,7 @@ int dispc_vp_bus_check(struct dispc_device *dispc, u32 hw_videoport, > return -EINVAL; > } > > - if (dispc->feat->vp_bus_type[hw_videoport] != DISPC_VP_OLDI && > + if (dispc->feat->output_port_bus_type[hw_videoport] != DISPC_PORT_OLDI && Hmm, so is the hw_videoport a vp index or an output index? Sounds like the former, so it's not right, even if at the moment they're identical. We need some kind of mapping between those. If the mapping can be changed (or just defined in the DT), I think we need a variable in struct dispc_device, which tells the output to which a videoport is connected to. Or vice versa, I'm not sure which direction we need more. If the mapping is always the same on all SoC (but I don't think so), we can have it in the feats. Also, I wonder if output_port is a good name as it has "port" in it (like video port), and it's a bit long-ish. Would just "output" be enough? We could, of course, shorten it to OP, but that looks odd to me =). > fmt->is_oldi_fmt) { > dev_dbg(dispc->dev, "%s: %s is not OLDI-port\n", > __func__, dispc->feat->vp_name[hw_videoport]); > @@ -955,7 +962,7 @@ void dispc_vp_prepare(struct dispc_device *dispc, u32 hw_videoport, > if (WARN_ON(!fmt)) > return; > > - if (dispc->feat->vp_bus_type[hw_videoport] == DISPC_VP_OLDI) { > + if (dispc->feat->output_port_bus_type[hw_videoport] == DISPC_PORT_OLDI) { > dispc_oldi_tx_power(dispc, true); > > dispc_enable_oldi(dispc, hw_videoport, fmt); > @@ -1014,7 +1021,7 @@ void dispc_vp_enable(struct dispc_device *dispc, u32 hw_videoport, > align = true; > > /* always use DE_HIGH for OLDI */ > - if (dispc->feat->vp_bus_type[hw_videoport] == DISPC_VP_OLDI) > + if (dispc->feat->output_port_bus_type[hw_videoport] == DISPC_PORT_OLDI) > ieo = false; > > dispc_vp_write(dispc, hw_videoport, DISPC_VP_POL_FREQ, > @@ -1040,7 +1047,7 @@ void dispc_vp_disable(struct dispc_device *dispc, u32 hw_videoport) > > void dispc_vp_unprepare(struct dispc_device *dispc, u32 hw_videoport) > { > - if (dispc->feat->vp_bus_type[hw_videoport] == DISPC_VP_OLDI) { > + if (dispc->feat->output_port_bus_type[hw_videoport] == DISPC_PORT_OLDI) { > dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, 0); > > dispc_oldi_tx_power(dispc, false); > @@ -1116,10 +1123,10 @@ enum drm_mode_status dispc_vp_mode_valid(struct dispc_device *dispc, > const struct drm_display_mode *mode) > { > u32 hsw, hfp, hbp, vsw, vfp, vbp; > - enum dispc_vp_bus_type bus_type; > + enum dispc_port_bus_type bus_type; > int max_pclk; > > - bus_type = dispc->feat->vp_bus_type[hw_videoport]; > + bus_type = dispc->feat->output_port_bus_type[hw_videoport]; > > max_pclk = dispc->feat->max_pclk_khz[bus_type]; > > diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h b/drivers/gpu/drm/tidss/tidss_dispc.h > index e49432f0abf5..30fb44158347 100644 > --- a/drivers/gpu/drm/tidss/tidss_dispc.h > +++ b/drivers/gpu/drm/tidss/tidss_dispc.h > @@ -50,11 +50,11 @@ struct dispc_errata { > bool i2000; /* DSS Does Not Support YUV Pixel Data Formats */ > }; > > -enum dispc_vp_bus_type { > - DISPC_VP_DPI, /* DPI output */ > - DISPC_VP_OLDI, /* OLDI (LVDS) output */ > - DISPC_VP_INTERNAL, /* SoC internal routing */ > - DISPC_VP_MAX_BUS_TYPE, > +enum dispc_port_bus_type { > + DISPC_PORT_DPI, /* DPI output */ > + DISPC_PORT_OLDI, /* OLDI (LVDS) output */ > + DISPC_PORT_INTERNAL, /* SoC internal routing */ > + DISPC_PORT_MAX_BUS_TYPE, Okay, so here you have just "port", not "output_port". In the DT, they're ports, so... Maybe we could use that name too, and for video port always use "vp". The current "hw_videoport" could be easily mistaken with "port". I don't recall why I chose to use "hw" prefix there. I think I wanted to separate it from some other videoport, but... I don't know what that "other" is =). Tomi
Hi Tomi, Thanks for the review! On 03-Feb-23 16:53, Tomi Valkeinen wrote: > On 25/01/2023 13:35, Aradhya Bhatia wrote: >> Make DSS Video Ports agnostic of output bus types. >> >> DSS controllers have had a 1-to-1 coupling between its VPs and its >> output ports. This no longer stands true for the new AM625 DSS. This >> coupling, hence, has been removed by renaming the 'vp_bus_type' to >> 'output_port_bus_type' because the VPs are essentially agnostic of the >> bus type and it is the output ports which have a bus type. >> >> The AM625 DSS has 2 VPs but requires 3 output ports to support its >> Dual-Link OLDI video output coming from a single VP. > > Not a biggie, but this sentence is a bit odd here at the end. Shouldn't > it be after the "...stands true for the new AM625 DSS."? Yes! It should be. Will make the edit. > >> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com> >> --- >> drivers/gpu/drm/tidss/tidss_dispc.c | 47 +++++++++++++++++------------ >> drivers/gpu/drm/tidss/tidss_dispc.h | 21 +++++++------ >> drivers/gpu/drm/tidss/tidss_drv.h | 5 +-- >> drivers/gpu/drm/tidss/tidss_irq.h | 2 +- >> drivers/gpu/drm/tidss/tidss_kms.c | 12 ++++---- >> 5 files changed, 48 insertions(+), 39 deletions(-) >> >> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c >> index 165365b515e1..c1c4faccbddc 100644 >> --- a/drivers/gpu/drm/tidss/tidss_dispc.c >> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c >> @@ -61,7 +61,7 @@ const struct dispc_features dispc_k2g_feats = { >> .min_pclk_khz = 4375, >> .max_pclk_khz = { >> - [DISPC_VP_DPI] = 150000, >> + [DISPC_PORT_DPI] = 150000, >> }, >> /* >> @@ -96,7 +96,6 @@ const struct dispc_features dispc_k2g_feats = { >> .vp_name = { "vp1" }, >> .ovr_name = { "ovr1" }, >> .vpclk_name = { "vp1" }, >> - .vp_bus_type = { DISPC_VP_DPI }, >> .vp_feat = { .color = { >> .has_ctm = true, >> @@ -109,6 +108,9 @@ const struct dispc_features dispc_k2g_feats = { >> .vid_name = { "vid1" }, >> .vid_lite = { false }, >> .vid_order = { 0 }, >> + >> + .num_output_ports = 1, >> + .output_port_bus_type = { DISPC_PORT_DPI }, >> }; > > Just thinking out loud, as these will get more complex in the future, > maybe we should finally group them with struct. E.g. we could define > struct array for vps, like (just hacky example): > > struct { > const char *name; > const char *clkname; > struct tidss_vp_feat feat; > } vps[TIDSS_MAX_PORTS]; > > and then use them as: > > .vps = { > { > .name = "kala", > .clkname = "kissa", > .feat.color.has_ctm = true, > }, { > .name = "kala2", > .clkname = "kissa2", > .feat.color.has_ctm = false, > }, > }, > > Perhaps something to try in the future. > Yes, agreed! Having that structure will tidy this up. I will keep this under future work. >> static const u16 tidss_am65x_common_regs[DISPC_COMMON_REG_TABLE_LEN] = { >> @@ -140,8 +142,8 @@ static const u16 tidss_am65x_common_regs[DISPC_COMMON_REG_TABLE_LEN] = { >> const struct dispc_features dispc_am65x_feats = { >> .max_pclk_khz = { >> - [DISPC_VP_DPI] = 165000, >> - [DISPC_VP_OLDI] = 165000, >> + [DISPC_PORT_DPI] = 165000, >> + [DISPC_PORT_OLDI] = 165000, >> }, >> .scaling = { >> @@ -171,7 +173,6 @@ const struct dispc_features dispc_am65x_feats = { >> .vp_name = { "vp1", "vp2" }, >> .ovr_name = { "ovr1", "ovr2" }, >> .vpclk_name = { "vp1", "vp2" }, >> - .vp_bus_type = { DISPC_VP_OLDI, DISPC_VP_DPI }, >> .vp_feat = { .color = { >> .has_ctm = true, >> @@ -185,6 +186,9 @@ const struct dispc_features dispc_am65x_feats = { >> .vid_name = { "vid", "vidl1" }, >> .vid_lite = { false, true, }, >> .vid_order = { 1, 0 }, >> + >> + .num_output_ports = 2, >> + .output_port_bus_type = { DISPC_PORT_OLDI, DISPC_PORT_DPI }, >> }; >> static const u16 tidss_j721e_common_regs[DISPC_COMMON_REG_TABLE_LEN] = { >> @@ -229,8 +233,8 @@ static const u16 tidss_j721e_common_regs[DISPC_COMMON_REG_TABLE_LEN] = { >> const struct dispc_features dispc_j721e_feats = { >> .max_pclk_khz = { >> - [DISPC_VP_DPI] = 170000, >> - [DISPC_VP_INTERNAL] = 600000, >> + [DISPC_PORT_DPI] = 170000, >> + [DISPC_PORT_INTERNAL] = 600000, >> }, >> .scaling = { >> @@ -260,9 +264,7 @@ const struct dispc_features dispc_j721e_feats = { >> .vp_name = { "vp1", "vp2", "vp3", "vp4" }, >> .ovr_name = { "ovr1", "ovr2", "ovr3", "ovr4" }, >> .vpclk_name = { "vp1", "vp2", "vp3", "vp4" }, >> - /* Currently hard coded VP routing (see dispc_initial_config()) */ >> - .vp_bus_type = { DISPC_VP_INTERNAL, DISPC_VP_DPI, >> - DISPC_VP_INTERNAL, DISPC_VP_DPI, }, >> + > > I think this line feed is extra. Okay! Will remove that from all SoC feat structs. > >> .vp_feat = { .color = { >> .has_ctm = true, >> .gamma_size = 1024, >> @@ -273,6 +275,11 @@ const struct dispc_features dispc_j721e_feats = { >> .vid_name = { "vid1", "vidl1", "vid2", "vidl2" }, >> .vid_lite = { 0, 1, 0, 1, }, >> .vid_order = { 1, 3, 0, 2 }, >> + >> + .num_output_ports = 4, >> + /* Currently hard coded VP routing (see dispc_initial_config()) */ >> + .output_port_bus_type = { DISPC_PORT_INTERNAL, DISPC_PORT_DPI, >> + DISPC_PORT_INTERNAL, DISPC_PORT_DPI, }, > > Indent doesn't look right (but it might be just because this is a diff). I may have missed indenting this. > >> }; >> static const u16 *dispc_common_regmap; >> @@ -287,12 +294,12 @@ struct dispc_device { >> void __iomem *base_common; >> void __iomem *base_vid[TIDSS_MAX_PLANES]; >> - void __iomem *base_ovr[TIDSS_MAX_PORTS]; >> - void __iomem *base_vp[TIDSS_MAX_PORTS]; >> + void __iomem *base_ovr[TIDSS_MAX_VPS]; >> + void __iomem *base_vp[TIDSS_MAX_VPS]; >> struct regmap *oldi_io_ctrl; >> - struct clk *vp_clk[TIDSS_MAX_PORTS]; >> + struct clk *vp_clk[TIDSS_MAX_VPS]; >> const struct dispc_features *feat; >> @@ -300,7 +307,7 @@ struct dispc_device { >> bool is_enabled; >> - struct dss_vp_data vp_data[TIDSS_MAX_PORTS]; >> + struct dss_vp_data vp_data[TIDSS_MAX_VPS]; >> u32 *fourccs; >> u32 num_fourccs; >> @@ -851,7 +858,7 @@ int dispc_vp_bus_check(struct dispc_device *dispc, u32 hw_videoport, >> return -EINVAL; >> } >> - if (dispc->feat->vp_bus_type[hw_videoport] != DISPC_VP_OLDI && >> + if (dispc->feat->output_port_bus_type[hw_videoport] != DISPC_PORT_OLDI && > > Hmm, so is the hw_videoport a vp index or an output index? Sounds like > the former, so it's not right, even if at the moment they're identical. > We need some kind of mapping between those. > It is indeed a vp index. And yes, I can come up with a mapping mechanism. > If the mapping can be changed (or just defined in the DT), I think we > need a variable in struct dispc_device, which tells the output to which > a videoport is connected to. Or vice versa, I'm not sure which direction > we need more. If the mapping is always the same on all SoC (but I don't > think so), we can have it in the feats. > As of now, the mapping is always same. But I would like to make is generalized for future. Hence, I am considering to keep the variable in struct dispc_device. My question though would be, how would one be able to find which kind of device is the port connected to, if it is connected to a bridge? For example, in case of panels, we have a "connector_type" variable in drm_panel which tells what kind of sink it is. But there is no such thing in drm_bridge. This is required because what if we can connect an videoport to either an LVDS/OLDI bridge or a DPI bridge. Also, implementing this might mean removal of the part of code which confirms that the panel's "connector_type" indeed expects what the VP can provide, unless there is a way to find out what the sink is before calling the drm_of_find_panel_or_bridge API. On the direction, the primary requirement of hw_videoport has been in the tidss_dispc.c file where the HW registers are getting configured. 'hw_videoport' is a frequently passed parameter in function calls. So, at a first glance the former option might makes more sense for direction, i.e. to have a variable which tells the output to which a videoport is connected to. And while, there is also the tidss_kms.c file, which deals with initializing encoders and attaching bridges. This is where the output_port is required more often. But I am yet to think of a case where the above direction could be an issue. > Also, I wonder if output_port is a good name as it has "port" in it > (like video port), and it's a bit long-ish. Would just "output" be > enough? We could, of course, shorten it to OP, but that looks odd to me =). > >> fmt->is_oldi_fmt) { >> dev_dbg(dispc->dev, "%s: %s is not OLDI-port\n", >> __func__, dispc->feat->vp_name[hw_videoport]); >> @@ -955,7 +962,7 @@ void dispc_vp_prepare(struct dispc_device *dispc, u32 hw_videoport, >> if (WARN_ON(!fmt)) >> return; >> - if (dispc->feat->vp_bus_type[hw_videoport] == DISPC_VP_OLDI) { >> + if (dispc->feat->output_port_bus_type[hw_videoport] == DISPC_PORT_OLDI) { >> dispc_oldi_tx_power(dispc, true); >> dispc_enable_oldi(dispc, hw_videoport, fmt); >> @@ -1014,7 +1021,7 @@ void dispc_vp_enable(struct dispc_device *dispc, u32 hw_videoport, >> align = true; >> /* always use DE_HIGH for OLDI */ >> - if (dispc->feat->vp_bus_type[hw_videoport] == DISPC_VP_OLDI) >> + if (dispc->feat->output_port_bus_type[hw_videoport] == DISPC_PORT_OLDI) >> ieo = false; >> dispc_vp_write(dispc, hw_videoport, DISPC_VP_POL_FREQ, >> @@ -1040,7 +1047,7 @@ void dispc_vp_disable(struct dispc_device *dispc, u32 hw_videoport) >> void dispc_vp_unprepare(struct dispc_device *dispc, u32 hw_videoport) >> { >> - if (dispc->feat->vp_bus_type[hw_videoport] == DISPC_VP_OLDI) { >> + if (dispc->feat->output_port_bus_type[hw_videoport] == DISPC_PORT_OLDI) { >> dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, 0); >> dispc_oldi_tx_power(dispc, false); >> @@ -1116,10 +1123,10 @@ enum drm_mode_status dispc_vp_mode_valid(struct dispc_device *dispc, >> const struct drm_display_mode *mode) >> { >> u32 hsw, hfp, hbp, vsw, vfp, vbp; >> - enum dispc_vp_bus_type bus_type; >> + enum dispc_port_bus_type bus_type; >> int max_pclk; >> - bus_type = dispc->feat->vp_bus_type[hw_videoport]; >> + bus_type = dispc->feat->output_port_bus_type[hw_videoport]; >> max_pclk = dispc->feat->max_pclk_khz[bus_type]; >> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h b/drivers/gpu/drm/tidss/tidss_dispc.h >> index e49432f0abf5..30fb44158347 100644 >> --- a/drivers/gpu/drm/tidss/tidss_dispc.h >> +++ b/drivers/gpu/drm/tidss/tidss_dispc.h >> @@ -50,11 +50,11 @@ struct dispc_errata { >> bool i2000; /* DSS Does Not Support YUV Pixel Data Formats */ >> }; >> -enum dispc_vp_bus_type { >> - DISPC_VP_DPI, /* DPI output */ >> - DISPC_VP_OLDI, /* OLDI (LVDS) output */ >> - DISPC_VP_INTERNAL, /* SoC internal routing */ >> - DISPC_VP_MAX_BUS_TYPE, >> +enum dispc_port_bus_type { >> + DISPC_PORT_DPI, /* DPI output */ >> + DISPC_PORT_OLDI, /* OLDI (LVDS) output */ >> + DISPC_PORT_INTERNAL, /* SoC internal routing */ >> + DISPC_PORT_MAX_BUS_TYPE, > > Okay, so here you have just "port", not "output_port". In the DT, > they're ports, so... Maybe we could use that name too, and for video > port always use "vp". The current "hw_videoport" could be easily > mistaken with "port". I see what you are saying and how somebody could confusre hw_videoport for a physical connection (i.e. port). I have always understoof hw_videoport to be a thing of the actual VP inside the SoC, but that may be because I have been working on this, and not just trying to understand the code from a high level. How about if I change the output_port to "out_port"? I am okay to keep "output" as the name to. I am saying this becuase I think, only keeping "port" might just confuse more, as you mentioned above. And then we can change "hw_videoport" to "vp_index", perhaps, or even keep that as it is? Becuase if we do have a VP structure in future like you suggested above, "vps" and "vp" would become a close overlap. For eg, "vps[vp]". How do these sound to you? > > I don't recall why I chose to use "hw" prefix there. I think I wanted to > separate it from some other videoport, but... I don't know what that > "other" is =). > Perhaps because just a "videoport" might be confused with a connector on the board, as in "HDMI port"? And the prefix might be a way to clarify that its the DSS controller's VP. =) Regards Aradhya
On 05/02/2023 15:08, Aradhya Bhatia wrote: > Hi Tomi, > > Thanks for the review! > > On 03-Feb-23 16:53, Tomi Valkeinen wrote: >> On 25/01/2023 13:35, Aradhya Bhatia wrote: >>> Make DSS Video Ports agnostic of output bus types. >>> >>> DSS controllers have had a 1-to-1 coupling between its VPs and its >>> output ports. This no longer stands true for the new AM625 DSS. This >>> coupling, hence, has been removed by renaming the 'vp_bus_type' to >>> 'output_port_bus_type' because the VPs are essentially agnostic of the >>> bus type and it is the output ports which have a bus type. >>> >>> The AM625 DSS has 2 VPs but requires 3 output ports to support its >>> Dual-Link OLDI video output coming from a single VP. >> >> Not a biggie, but this sentence is a bit odd here at the end. Shouldn't >> it be after the "...stands true for the new AM625 DSS."? > > Yes! It should be. Will make the edit. > >> >>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com> >>> --- >>> drivers/gpu/drm/tidss/tidss_dispc.c | 47 +++++++++++++++++------------ >>> drivers/gpu/drm/tidss/tidss_dispc.h | 21 +++++++------ >>> drivers/gpu/drm/tidss/tidss_drv.h | 5 +-- >>> drivers/gpu/drm/tidss/tidss_irq.h | 2 +- >>> drivers/gpu/drm/tidss/tidss_kms.c | 12 ++++---- >>> 5 files changed, 48 insertions(+), 39 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c >>> index 165365b515e1..c1c4faccbddc 100644 >>> --- a/drivers/gpu/drm/tidss/tidss_dispc.c >>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c >>> @@ -61,7 +61,7 @@ const struct dispc_features dispc_k2g_feats = { >>> .min_pclk_khz = 4375, >>> .max_pclk_khz = { >>> - [DISPC_VP_DPI] = 150000, >>> + [DISPC_PORT_DPI] = 150000, >>> }, >>> /* >>> @@ -96,7 +96,6 @@ const struct dispc_features dispc_k2g_feats = { >>> .vp_name = { "vp1" }, >>> .ovr_name = { "ovr1" }, >>> .vpclk_name = { "vp1" }, >>> - .vp_bus_type = { DISPC_VP_DPI }, >>> .vp_feat = { .color = { >>> .has_ctm = true, >>> @@ -109,6 +108,9 @@ const struct dispc_features dispc_k2g_feats = { >>> .vid_name = { "vid1" }, >>> .vid_lite = { false }, >>> .vid_order = { 0 }, >>> + >>> + .num_output_ports = 1, >>> + .output_port_bus_type = { DISPC_PORT_DPI }, >>> }; >> >> Just thinking out loud, as these will get more complex in the future, >> maybe we should finally group them with struct. E.g. we could define >> struct array for vps, like (just hacky example): >> >> struct { >> const char *name; >> const char *clkname; >> struct tidss_vp_feat feat; >> } vps[TIDSS_MAX_PORTS]; >> >> and then use them as: >> >> .vps = { >> { >> .name = "kala", >> .clkname = "kissa", >> .feat.color.has_ctm = true, >> }, { >> .name = "kala2", >> .clkname = "kissa2", >> .feat.color.has_ctm = false, >> }, >> }, >> >> Perhaps something to try in the future. >> > > Yes, agreed! Having that structure will tidy this up. > I will keep this under future work. > >>> static const u16 tidss_am65x_common_regs[DISPC_COMMON_REG_TABLE_LEN] = { >>> @@ -140,8 +142,8 @@ static const u16 tidss_am65x_common_regs[DISPC_COMMON_REG_TABLE_LEN] = { >>> const struct dispc_features dispc_am65x_feats = { >>> .max_pclk_khz = { >>> - [DISPC_VP_DPI] = 165000, >>> - [DISPC_VP_OLDI] = 165000, >>> + [DISPC_PORT_DPI] = 165000, >>> + [DISPC_PORT_OLDI] = 165000, >>> }, >>> .scaling = { >>> @@ -171,7 +173,6 @@ const struct dispc_features dispc_am65x_feats = { >>> .vp_name = { "vp1", "vp2" }, >>> .ovr_name = { "ovr1", "ovr2" }, >>> .vpclk_name = { "vp1", "vp2" }, >>> - .vp_bus_type = { DISPC_VP_OLDI, DISPC_VP_DPI }, >>> .vp_feat = { .color = { >>> .has_ctm = true, >>> @@ -185,6 +186,9 @@ const struct dispc_features dispc_am65x_feats = { >>> .vid_name = { "vid", "vidl1" }, >>> .vid_lite = { false, true, }, >>> .vid_order = { 1, 0 }, >>> + >>> + .num_output_ports = 2, >>> + .output_port_bus_type = { DISPC_PORT_OLDI, DISPC_PORT_DPI }, >>> }; >>> static const u16 tidss_j721e_common_regs[DISPC_COMMON_REG_TABLE_LEN] = { >>> @@ -229,8 +233,8 @@ static const u16 tidss_j721e_common_regs[DISPC_COMMON_REG_TABLE_LEN] = { >>> const struct dispc_features dispc_j721e_feats = { >>> .max_pclk_khz = { >>> - [DISPC_VP_DPI] = 170000, >>> - [DISPC_VP_INTERNAL] = 600000, >>> + [DISPC_PORT_DPI] = 170000, >>> + [DISPC_PORT_INTERNAL] = 600000, >>> }, >>> .scaling = { >>> @@ -260,9 +264,7 @@ const struct dispc_features dispc_j721e_feats = { >>> .vp_name = { "vp1", "vp2", "vp3", "vp4" }, >>> .ovr_name = { "ovr1", "ovr2", "ovr3", "ovr4" }, >>> .vpclk_name = { "vp1", "vp2", "vp3", "vp4" }, >>> - /* Currently hard coded VP routing (see dispc_initial_config()) */ >>> - .vp_bus_type = { DISPC_VP_INTERNAL, DISPC_VP_DPI, >>> - DISPC_VP_INTERNAL, DISPC_VP_DPI, }, >>> + >> >> I think this line feed is extra. > > Okay! Will remove that from all SoC feat structs. > >> >>> .vp_feat = { .color = { >>> .has_ctm = true, >>> .gamma_size = 1024, >>> @@ -273,6 +275,11 @@ const struct dispc_features dispc_j721e_feats = { >>> .vid_name = { "vid1", "vidl1", "vid2", "vidl2" }, >>> .vid_lite = { 0, 1, 0, 1, }, >>> .vid_order = { 1, 3, 0, 2 }, >>> + >>> + .num_output_ports = 4, >>> + /* Currently hard coded VP routing (see dispc_initial_config()) */ >>> + .output_port_bus_type = { DISPC_PORT_INTERNAL, DISPC_PORT_DPI, >>> + DISPC_PORT_INTERNAL, DISPC_PORT_DPI, }, >> >> Indent doesn't look right (but it might be just because this is a diff). > > I may have missed indenting this. > >> >>> }; >>> static const u16 *dispc_common_regmap; >>> @@ -287,12 +294,12 @@ struct dispc_device { >>> void __iomem *base_common; >>> void __iomem *base_vid[TIDSS_MAX_PLANES]; >>> - void __iomem *base_ovr[TIDSS_MAX_PORTS]; >>> - void __iomem *base_vp[TIDSS_MAX_PORTS]; >>> + void __iomem *base_ovr[TIDSS_MAX_VPS]; >>> + void __iomem *base_vp[TIDSS_MAX_VPS]; >>> struct regmap *oldi_io_ctrl; >>> - struct clk *vp_clk[TIDSS_MAX_PORTS]; >>> + struct clk *vp_clk[TIDSS_MAX_VPS]; >>> const struct dispc_features *feat; >>> @@ -300,7 +307,7 @@ struct dispc_device { >>> bool is_enabled; >>> - struct dss_vp_data vp_data[TIDSS_MAX_PORTS]; >>> + struct dss_vp_data vp_data[TIDSS_MAX_VPS]; >>> u32 *fourccs; >>> u32 num_fourccs; >>> @@ -851,7 +858,7 @@ int dispc_vp_bus_check(struct dispc_device *dispc, u32 hw_videoport, >>> return -EINVAL; >>> } >>> - if (dispc->feat->vp_bus_type[hw_videoport] != DISPC_VP_OLDI && >>> + if (dispc->feat->output_port_bus_type[hw_videoport] != DISPC_PORT_OLDI && >> >> Hmm, so is the hw_videoport a vp index or an output index? Sounds like >> the former, so it's not right, even if at the moment they're identical. >> We need some kind of mapping between those. >> > > It is indeed a vp index. And yes, I can come up with a mapping mechanism. > >> If the mapping can be changed (or just defined in the DT), I think we >> need a variable in struct dispc_device, which tells the output to which >> a videoport is connected to. Or vice versa, I'm not sure which direction >> we need more. If the mapping is always the same on all SoC (but I don't >> think so), we can have it in the feats. >> > > As of now, the mapping is always same. But I would like to make is > generalized for future. Hence, I am considering to keep the variable in > struct dispc_device. > > My question though would be, how would one be able to find which kind > of device is the port connected to, if it is connected to a bridge? For > example, in case of panels, we have a "connector_type" variable in > drm_panel which tells what kind of sink it is. But there is no such > thing in drm_bridge. > > This is required because what if we can connect an videoport to either > an LVDS/OLDI bridge or a DPI bridge. The connector type shouldn't matter. The DSS has VPs and outputs. The VPs are "generic" and identical to each other, except in their possible connections to the outputs. The outputs, at least at the moment, are DPI, LVDS and internal, where internal is basically just DPI. Those are the three different cases we are interested in within the dss driver, right? Does it matter where the DPI or LVDS output goes? So what I'm saying is that the DSS device tree data should already define what kind of configuration we need, and there's no need to look further into the panel/bridge nodes. > Also, implementing this might mean removal of the part of code which > confirms that the panel's "connector_type" indeed expects what the VP > can provide, unless there is a way to find out what the sink is before > calling the drm_of_find_panel_or_bridge API. Hmm, well, each DSS output (port in DT) is of a certain type, so we should be able to validate that the output and the panel's connector_type match. > On the direction, the primary requirement of hw_videoport has been in > the tidss_dispc.c file where the HW registers are getting configured. > 'hw_videoport' is a frequently passed parameter in function calls. So, > at a first glance the former option might makes more sense for > direction, i.e. to have a variable which tells the output to which a > videoport is connected to. Makes sense. > And while, there is also the tidss_kms.c file, which deals with > initializing encoders and attaching bridges. This is where the > output_port is required more often. But I am yet to think of a case > where the above direction could be an issue. > > >> Also, I wonder if output_port is a good name as it has "port" in it >> (like video port), and it's a bit long-ish. Would just "output" be >> enough? We could, of course, shorten it to OP, but that looks odd to me =). >> > >>> fmt->is_oldi_fmt) { >>> dev_dbg(dispc->dev, "%s: %s is not OLDI-port\n", >>> __func__, dispc->feat->vp_name[hw_videoport]); >>> @@ -955,7 +962,7 @@ void dispc_vp_prepare(struct dispc_device *dispc, u32 hw_videoport, >>> if (WARN_ON(!fmt)) >>> return; >>> - if (dispc->feat->vp_bus_type[hw_videoport] == DISPC_VP_OLDI) { >>> + if (dispc->feat->output_port_bus_type[hw_videoport] == DISPC_PORT_OLDI) { >>> dispc_oldi_tx_power(dispc, true); >>> dispc_enable_oldi(dispc, hw_videoport, fmt); >>> @@ -1014,7 +1021,7 @@ void dispc_vp_enable(struct dispc_device *dispc, u32 hw_videoport, >>> align = true; >>> /* always use DE_HIGH for OLDI */ >>> - if (dispc->feat->vp_bus_type[hw_videoport] == DISPC_VP_OLDI) >>> + if (dispc->feat->output_port_bus_type[hw_videoport] == DISPC_PORT_OLDI) >>> ieo = false; >>> dispc_vp_write(dispc, hw_videoport, DISPC_VP_POL_FREQ, >>> @@ -1040,7 +1047,7 @@ void dispc_vp_disable(struct dispc_device *dispc, u32 hw_videoport) >>> void dispc_vp_unprepare(struct dispc_device *dispc, u32 hw_videoport) >>> { >>> - if (dispc->feat->vp_bus_type[hw_videoport] == DISPC_VP_OLDI) { >>> + if (dispc->feat->output_port_bus_type[hw_videoport] == DISPC_PORT_OLDI) { >>> dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, 0); >>> dispc_oldi_tx_power(dispc, false); >>> @@ -1116,10 +1123,10 @@ enum drm_mode_status dispc_vp_mode_valid(struct dispc_device *dispc, >>> const struct drm_display_mode *mode) >>> { >>> u32 hsw, hfp, hbp, vsw, vfp, vbp; >>> - enum dispc_vp_bus_type bus_type; >>> + enum dispc_port_bus_type bus_type; >>> int max_pclk; >>> - bus_type = dispc->feat->vp_bus_type[hw_videoport]; >>> + bus_type = dispc->feat->output_port_bus_type[hw_videoport]; >>> max_pclk = dispc->feat->max_pclk_khz[bus_type]; >>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h b/drivers/gpu/drm/tidss/tidss_dispc.h >>> index e49432f0abf5..30fb44158347 100644 >>> --- a/drivers/gpu/drm/tidss/tidss_dispc.h >>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.h >>> @@ -50,11 +50,11 @@ struct dispc_errata { >>> bool i2000; /* DSS Does Not Support YUV Pixel Data Formats */ >>> }; >>> -enum dispc_vp_bus_type { >>> - DISPC_VP_DPI, /* DPI output */ >>> - DISPC_VP_OLDI, /* OLDI (LVDS) output */ >>> - DISPC_VP_INTERNAL, /* SoC internal routing */ >>> - DISPC_VP_MAX_BUS_TYPE, >>> +enum dispc_port_bus_type { >>> + DISPC_PORT_DPI, /* DPI output */ >>> + DISPC_PORT_OLDI, /* OLDI (LVDS) output */ >>> + DISPC_PORT_INTERNAL, /* SoC internal routing */ >>> + DISPC_PORT_MAX_BUS_TYPE, >> >> Okay, so here you have just "port", not "output_port". In the DT, >> they're ports, so... Maybe we could use that name too, and for video >> port always use "vp". The current "hw_videoport" could be easily >> mistaken with "port". > > I see what you are saying and how somebody could confusre hw_videoport > for a physical connection (i.e. port). I have always understoof > hw_videoport to be a thing of the actual VP inside the SoC, but that may > be because I have been working on this, and not just trying to > understand the code from a high level. > > How about if I change the output_port to "out_port"? I am okay to keep > "output" as the name to. I am saying this becuase I think, only keeping > "port" might just confuse more, as you mentioned above. Yes, I agree "port" is not good. Other than that, no strong opinions. Whatever name you pick, someone will find it confusing ;). Just keep it consistent, so that all enums, parameters, etc. use it a consistent manner. If I had to choose, I'd go with the "output", but I'm fine with other names too. > And then we can change "hw_videoport" to "vp_index", perhaps, or even > keep that as it is? Becuase if we do have a VP structure in future > like you suggested above, "vps" and "vp" would become a close overlap. > For eg, "vps[vp]". That is true. I think that was the reason I chose hw_videoport instead of videoport or vp, although vps[hw_videoport] is only barely better. vp_index is ok for me, or maybe vp_idx or vp_num to have it a bit shorter. Tomi
On 06-Feb-23 18:35, Tomi Valkeinen wrote: > On 05/02/2023 15:08, Aradhya Bhatia wrote: >> Hi Tomi, >> >> Thanks for the review! >> >> On 03-Feb-23 16:53, Tomi Valkeinen wrote: >>> On 25/01/2023 13:35, Aradhya Bhatia wrote: >>>> Make DSS Video Ports agnostic of output bus types. >>>> >>>> DSS controllers have had a 1-to-1 coupling between its VPs and its >>>> output ports. This no longer stands true for the new AM625 DSS. This >>>> coupling, hence, has been removed by renaming the 'vp_bus_type' to >>>> 'output_port_bus_type' because the VPs are essentially agnostic of the >>>> bus type and it is the output ports which have a bus type. >>>> >>>> The AM625 DSS has 2 VPs but requires 3 output ports to support its >>>> Dual-Link OLDI video output coming from a single VP. >>> >>> Not a biggie, but this sentence is a bit odd here at the end. Shouldn't >>> it be after the "...stands true for the new AM625 DSS."? >> >> Yes! It should be. Will make the edit. >> >>> >>>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com> >>>> --- >>>> drivers/gpu/drm/tidss/tidss_dispc.c | 47 +++++++++++++++++------------ >>>> drivers/gpu/drm/tidss/tidss_dispc.h | 21 +++++++------ >>>> drivers/gpu/drm/tidss/tidss_drv.h | 5 +-- >>>> drivers/gpu/drm/tidss/tidss_irq.h | 2 +- >>>> drivers/gpu/drm/tidss/tidss_kms.c | 12 ++++---- >>>> 5 files changed, 48 insertions(+), 39 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c >>>> index 165365b515e1..c1c4faccbddc 100644 >>>> --- a/drivers/gpu/drm/tidss/tidss_dispc.c >>>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c >>>> @@ -61,7 +61,7 @@ const struct dispc_features dispc_k2g_feats = { >>>> .min_pclk_khz = 4375, >>>> .max_pclk_khz = { >>>> - [DISPC_VP_DPI] = 150000, >>>> + [DISPC_PORT_DPI] = 150000, >>>> }, >>>> /* >>>> @@ -96,7 +96,6 @@ const struct dispc_features dispc_k2g_feats = { >>>> .vp_name = { "vp1" }, >>>> .ovr_name = { "ovr1" }, >>>> .vpclk_name = { "vp1" }, >>>> - .vp_bus_type = { DISPC_VP_DPI }, >>>> .vp_feat = { .color = { >>>> .has_ctm = true, >>>> @@ -109,6 +108,9 @@ const struct dispc_features dispc_k2g_feats = { >>>> .vid_name = { "vid1" }, >>>> .vid_lite = { false }, >>>> .vid_order = { 0 }, >>>> + >>>> + .num_output_ports = 1, >>>> + .output_port_bus_type = { DISPC_PORT_DPI }, >>>> }; >>> >>> Just thinking out loud, as these will get more complex in the future, >>> maybe we should finally group them with struct. E.g. we could define >>> struct array for vps, like (just hacky example): >>> >>> struct { >>> const char *name; >>> const char *clkname; >>> struct tidss_vp_feat feat; >>> } vps[TIDSS_MAX_PORTS]; >>> >>> and then use them as: >>> >>> .vps = { >>> { >>> .name = "kala", >>> .clkname = "kissa", >>> .feat.color.has_ctm = true, >>> }, { >>> .name = "kala2", >>> .clkname = "kissa2", >>> .feat.color.has_ctm = false, >>> }, >>> }, >>> >>> Perhaps something to try in the future. >>> >> >> Yes, agreed! Having that structure will tidy this up. >> I will keep this under future work. >> >>>> static const u16 tidss_am65x_common_regs[DISPC_COMMON_REG_TABLE_LEN] = { >>>> @@ -140,8 +142,8 @@ static const u16 tidss_am65x_common_regs[DISPC_COMMON_REG_TABLE_LEN] = { >>>> const struct dispc_features dispc_am65x_feats = { >>>> .max_pclk_khz = { >>>> - [DISPC_VP_DPI] = 165000, >>>> - [DISPC_VP_OLDI] = 165000, >>>> + [DISPC_PORT_DPI] = 165000, >>>> + [DISPC_PORT_OLDI] = 165000, >>>> }, >>>> .scaling = { >>>> @@ -171,7 +173,6 @@ const struct dispc_features dispc_am65x_feats = { >>>> .vp_name = { "vp1", "vp2" }, >>>> .ovr_name = { "ovr1", "ovr2" }, >>>> .vpclk_name = { "vp1", "vp2" }, >>>> - .vp_bus_type = { DISPC_VP_OLDI, DISPC_VP_DPI }, >>>> .vp_feat = { .color = { >>>> .has_ctm = true, >>>> @@ -185,6 +186,9 @@ const struct dispc_features dispc_am65x_feats = { >>>> .vid_name = { "vid", "vidl1" }, >>>> .vid_lite = { false, true, }, >>>> .vid_order = { 1, 0 }, >>>> + >>>> + .num_output_ports = 2, >>>> + .output_port_bus_type = { DISPC_PORT_OLDI, DISPC_PORT_DPI }, >>>> }; >>>> static const u16 tidss_j721e_common_regs[DISPC_COMMON_REG_TABLE_LEN] = { >>>> @@ -229,8 +233,8 @@ static const u16 tidss_j721e_common_regs[DISPC_COMMON_REG_TABLE_LEN] = { >>>> const struct dispc_features dispc_j721e_feats = { >>>> .max_pclk_khz = { >>>> - [DISPC_VP_DPI] = 170000, >>>> - [DISPC_VP_INTERNAL] = 600000, >>>> + [DISPC_PORT_DPI] = 170000, >>>> + [DISPC_PORT_INTERNAL] = 600000, >>>> }, >>>> .scaling = { >>>> @@ -260,9 +264,7 @@ const struct dispc_features dispc_j721e_feats = { >>>> .vp_name = { "vp1", "vp2", "vp3", "vp4" }, >>>> .ovr_name = { "ovr1", "ovr2", "ovr3", "ovr4" }, >>>> .vpclk_name = { "vp1", "vp2", "vp3", "vp4" }, >>>> - /* Currently hard coded VP routing (see dispc_initial_config()) */ >>>> - .vp_bus_type = { DISPC_VP_INTERNAL, DISPC_VP_DPI, >>>> - DISPC_VP_INTERNAL, DISPC_VP_DPI, }, >>>> + >>> >>> I think this line feed is extra. >> >> Okay! Will remove that from all SoC feat structs. >> >>> >>>> .vp_feat = { .color = { >>>> .has_ctm = true, >>>> .gamma_size = 1024, >>>> @@ -273,6 +275,11 @@ const struct dispc_features dispc_j721e_feats = { >>>> .vid_name = { "vid1", "vidl1", "vid2", "vidl2" }, >>>> .vid_lite = { 0, 1, 0, 1, }, >>>> .vid_order = { 1, 3, 0, 2 }, >>>> + >>>> + .num_output_ports = 4, >>>> + /* Currently hard coded VP routing (see dispc_initial_config()) */ >>>> + .output_port_bus_type = { DISPC_PORT_INTERNAL, DISPC_PORT_DPI, >>>> + DISPC_PORT_INTERNAL, DISPC_PORT_DPI, }, >>> >>> Indent doesn't look right (but it might be just because this is a diff). >> >> I may have missed indenting this. >> >>> >>>> }; >>>> static const u16 *dispc_common_regmap; >>>> @@ -287,12 +294,12 @@ struct dispc_device { >>>> void __iomem *base_common; >>>> void __iomem *base_vid[TIDSS_MAX_PLANES]; >>>> - void __iomem *base_ovr[TIDSS_MAX_PORTS]; >>>> - void __iomem *base_vp[TIDSS_MAX_PORTS]; >>>> + void __iomem *base_ovr[TIDSS_MAX_VPS]; >>>> + void __iomem *base_vp[TIDSS_MAX_VPS]; >>>> struct regmap *oldi_io_ctrl; >>>> - struct clk *vp_clk[TIDSS_MAX_PORTS]; >>>> + struct clk *vp_clk[TIDSS_MAX_VPS]; >>>> const struct dispc_features *feat; >>>> @@ -300,7 +307,7 @@ struct dispc_device { >>>> bool is_enabled; >>>> - struct dss_vp_data vp_data[TIDSS_MAX_PORTS]; >>>> + struct dss_vp_data vp_data[TIDSS_MAX_VPS]; >>>> u32 *fourccs; >>>> u32 num_fourccs; >>>> @@ -851,7 +858,7 @@ int dispc_vp_bus_check(struct dispc_device *dispc, u32 hw_videoport, >>>> return -EINVAL; >>>> } >>>> - if (dispc->feat->vp_bus_type[hw_videoport] != DISPC_VP_OLDI && >>>> + if (dispc->feat->output_port_bus_type[hw_videoport] != DISPC_PORT_OLDI && >>> >>> Hmm, so is the hw_videoport a vp index or an output index? Sounds like >>> the former, so it's not right, even if at the moment they're identical. >>> We need some kind of mapping between those. >>> >> >> It is indeed a vp index. And yes, I can come up with a mapping mechanism. >> >>> If the mapping can be changed (or just defined in the DT), I think we >>> need a variable in struct dispc_device, which tells the output to which >>> a videoport is connected to. Or vice versa, I'm not sure which direction >>> we need more. If the mapping is always the same on all SoC (but I don't >>> think so), we can have it in the feats. >>> >> >> As of now, the mapping is always same. But I would like to make is >> generalized for future. Hence, I am considering to keep the variable in >> struct dispc_device. >> >> My question though would be, how would one be able to find which kind >> of device is the port connected to, if it is connected to a bridge? For >> example, in case of panels, we have a "connector_type" variable in >> drm_panel which tells what kind of sink it is. But there is no such >> thing in drm_bridge. >> >> This is required because what if we can connect an videoport to either >> an LVDS/OLDI bridge or a DPI bridge. > > The connector type shouldn't matter. > > The DSS has VPs and outputs. The VPs are "generic" and identical to each > other, except in their possible connections to the outputs. The outputs, > at least at the moment, are DPI, LVDS and internal, where internal is > basically just DPI. > > Those are the three different cases we are interested in within the dss > driver, right? Does it matter where the DPI or LVDS output goes? > I believe it does. =) While the VPs do always transmit DPI signals, the code in tidss_dispc.c uses the information of the bus connected at the endpoint to configure the OLDI parameters, and to turn OLDI IOs on and off in dispc_vp_(prepare/unprepare). Up until now, the outputs have been fixed (VP0 -> OLDI, VP1 -> DPI), and the code used the enum dispc_vp_bus_type to differntiate between LVDS or DPI requirements. But for a general case where output from VP0 could either use the OLDI TXes and send out LVDS signals OR bypass the OLDI TXes and send out DPI signals directly, we would need a mechanism to find out which sink is present at the end, LVDS or DPI. I assumed, with that mechanism, we could (re)configure the vp-to-output mapping, which then would be used in the various places in tidss_dispc.c. > So what I'm saying is that the DSS device tree data should already > define what kind of configuration we need, and there's no need to look > further into the panel/bridge nodes. > >> Also, implementing this might mean removal of the part of code which >> confirms that the panel's "connector_type" indeed expects what the VP >> can provide, unless there is a way to find out what the sink is before >> calling the drm_of_find_panel_or_bridge API. > > Hmm, well, each DSS output (port in DT) is of a certain type, so we > should be able to validate that the output and the panel's > connector_type match. > >> On the direction, the primary requirement of hw_videoport has been in >> the tidss_dispc.c file where the HW registers are getting configured. >> 'hw_videoport' is a frequently passed parameter in function calls. So, >> at a first glance the former option might makes more sense for >> direction, i.e. to have a variable which tells the output to which a >> videoport is connected to. > > Makes sense. > >> And while, there is also the tidss_kms.c file, which deals with >> initializing encoders and attaching bridges. This is where the >> output_port is required more often. But I am yet to think of a case >> where the above direction could be an issue. >> >> >>> Also, I wonder if output_port is a good name as it has "port" in it >>> (like video port), and it's a bit long-ish. Would just "output" be >>> enough? We could, of course, shorten it to OP, but that looks odd to me =). >>> >> >>>> fmt->is_oldi_fmt) { >>>> dev_dbg(dispc->dev, "%s: %s is not OLDI-port\n", >>>> __func__, dispc->feat->vp_name[hw_videoport]); >>>> @@ -955,7 +962,7 @@ void dispc_vp_prepare(struct dispc_device *dispc, u32 hw_videoport, >>>> if (WARN_ON(!fmt)) >>>> return; >>>> - if (dispc->feat->vp_bus_type[hw_videoport] == DISPC_VP_OLDI) { >>>> + if (dispc->feat->output_port_bus_type[hw_videoport] == DISPC_PORT_OLDI) { >>>> dispc_oldi_tx_power(dispc, true); >>>> dispc_enable_oldi(dispc, hw_videoport, fmt); >>>> @@ -1014,7 +1021,7 @@ void dispc_vp_enable(struct dispc_device *dispc, u32 hw_videoport, >>>> align = true; >>>> /* always use DE_HIGH for OLDI */ >>>> - if (dispc->feat->vp_bus_type[hw_videoport] == DISPC_VP_OLDI) >>>> + if (dispc->feat->output_port_bus_type[hw_videoport] == DISPC_PORT_OLDI) >>>> ieo = false; >>>> dispc_vp_write(dispc, hw_videoport, DISPC_VP_POL_FREQ, >>>> @@ -1040,7 +1047,7 @@ void dispc_vp_disable(struct dispc_device *dispc, u32 hw_videoport) >>>> void dispc_vp_unprepare(struct dispc_device *dispc, u32 hw_videoport) >>>> { >>>> - if (dispc->feat->vp_bus_type[hw_videoport] == DISPC_VP_OLDI) { >>>> + if (dispc->feat->output_port_bus_type[hw_videoport] == DISPC_PORT_OLDI) { >>>> dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, 0); >>>> dispc_oldi_tx_power(dispc, false); >>>> @@ -1116,10 +1123,10 @@ enum drm_mode_status dispc_vp_mode_valid(struct dispc_device *dispc, >>>> const struct drm_display_mode *mode) >>>> { >>>> u32 hsw, hfp, hbp, vsw, vfp, vbp; >>>> - enum dispc_vp_bus_type bus_type; >>>> + enum dispc_port_bus_type bus_type; >>>> int max_pclk; >>>> - bus_type = dispc->feat->vp_bus_type[hw_videoport]; >>>> + bus_type = dispc->feat->output_port_bus_type[hw_videoport]; >>>> max_pclk = dispc->feat->max_pclk_khz[bus_type]; >>>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h b/drivers/gpu/drm/tidss/tidss_dispc.h >>>> index e49432f0abf5..30fb44158347 100644 >>>> --- a/drivers/gpu/drm/tidss/tidss_dispc.h >>>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.h >>>> @@ -50,11 +50,11 @@ struct dispc_errata { >>>> bool i2000; /* DSS Does Not Support YUV Pixel Data Formats */ >>>> }; >>>> -enum dispc_vp_bus_type { >>>> - DISPC_VP_DPI, /* DPI output */ >>>> - DISPC_VP_OLDI, /* OLDI (LVDS) output */ >>>> - DISPC_VP_INTERNAL, /* SoC internal routing */ >>>> - DISPC_VP_MAX_BUS_TYPE, >>>> +enum dispc_port_bus_type { >>>> + DISPC_PORT_DPI, /* DPI output */ >>>> + DISPC_PORT_OLDI, /* OLDI (LVDS) output */ >>>> + DISPC_PORT_INTERNAL, /* SoC internal routing */ >>>> + DISPC_PORT_MAX_BUS_TYPE, >>> >>> Okay, so here you have just "port", not "output_port". In the DT, >>> they're ports, so... Maybe we could use that name too, and for video >>> port always use "vp". The current "hw_videoport" could be easily >>> mistaken with "port". >> >> I see what you are saying and how somebody could confusre hw_videoport >> for a physical connection (i.e. port). I have always understoof >> hw_videoport to be a thing of the actual VP inside the SoC, but that may >> be because I have been working on this, and not just trying to >> understand the code from a high level. >> >> How about if I change the output_port to "out_port"? I am okay to keep >> "output" as the name to. I am saying this becuase I think, only keeping >> "port" might just confuse more, as you mentioned above. > > Yes, I agree "port" is not good. Other than that, no strong opinions. > Whatever name you pick, someone will find it confusing ;). Just keep it > consistent, so that all enums, parameters, etc. use it a consistent > manner. If I had to choose, I'd go with the "output", but I'm fine with > other names too. > Alright! I am good with using "output". >> And then we can change "hw_videoport" to "vp_index", perhaps, or even >> keep that as it is? Becuase if we do have a VP structure in future >> like you suggested above, "vps" and "vp" would become a close overlap. >> For eg, "vps[vp]". > > That is true. I think that was the reason I chose hw_videoport instead > of videoport or vp, although vps[hw_videoport] is only barely better. > > vp_index is ok for me, or maybe vp_idx or vp_num to have it a bit shorter. > "vp_idx" seems better! Regards Aradhya
On 06/02/2023 19:34, Aradhya Bhatia wrote: > > On 06-Feb-23 18:35, Tomi Valkeinen wrote: >> On 05/02/2023 15:08, Aradhya Bhatia wrote: >>> Hi Tomi, >>> >>> Thanks for the review! >>> >>> On 03-Feb-23 16:53, Tomi Valkeinen wrote: >>>> On 25/01/2023 13:35, Aradhya Bhatia wrote: >>>>> Make DSS Video Ports agnostic of output bus types. >>>>> >>>>> DSS controllers have had a 1-to-1 coupling between its VPs and its >>>>> output ports. This no longer stands true for the new AM625 DSS. This >>>>> coupling, hence, has been removed by renaming the 'vp_bus_type' to >>>>> 'output_port_bus_type' because the VPs are essentially agnostic of the >>>>> bus type and it is the output ports which have a bus type. >>>>> >>>>> The AM625 DSS has 2 VPs but requires 3 output ports to support its >>>>> Dual-Link OLDI video output coming from a single VP. >>>> >>>> Not a biggie, but this sentence is a bit odd here at the end. Shouldn't >>>> it be after the "...stands true for the new AM625 DSS."? >>> >>> Yes! It should be. Will make the edit. >>> >>>> >>>>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com> >>>>> --- >>>>> drivers/gpu/drm/tidss/tidss_dispc.c | 47 +++++++++++++++++------------ >>>>> drivers/gpu/drm/tidss/tidss_dispc.h | 21 +++++++------ >>>>> drivers/gpu/drm/tidss/tidss_drv.h | 5 +-- >>>>> drivers/gpu/drm/tidss/tidss_irq.h | 2 +- >>>>> drivers/gpu/drm/tidss/tidss_kms.c | 12 ++++---- >>>>> 5 files changed, 48 insertions(+), 39 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c >>>>> index 165365b515e1..c1c4faccbddc 100644 >>>>> --- a/drivers/gpu/drm/tidss/tidss_dispc.c >>>>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c >>>>> @@ -61,7 +61,7 @@ const struct dispc_features dispc_k2g_feats = { >>>>> .min_pclk_khz = 4375, >>>>> .max_pclk_khz = { >>>>> - [DISPC_VP_DPI] = 150000, >>>>> + [DISPC_PORT_DPI] = 150000, >>>>> }, >>>>> /* >>>>> @@ -96,7 +96,6 @@ const struct dispc_features dispc_k2g_feats = { >>>>> .vp_name = { "vp1" }, >>>>> .ovr_name = { "ovr1" }, >>>>> .vpclk_name = { "vp1" }, >>>>> - .vp_bus_type = { DISPC_VP_DPI }, >>>>> .vp_feat = { .color = { >>>>> .has_ctm = true, >>>>> @@ -109,6 +108,9 @@ const struct dispc_features dispc_k2g_feats = { >>>>> .vid_name = { "vid1" }, >>>>> .vid_lite = { false }, >>>>> .vid_order = { 0 }, >>>>> + >>>>> + .num_output_ports = 1, >>>>> + .output_port_bus_type = { DISPC_PORT_DPI }, >>>>> }; >>>> >>>> Just thinking out loud, as these will get more complex in the future, >>>> maybe we should finally group them with struct. E.g. we could define >>>> struct array for vps, like (just hacky example): >>>> >>>> struct { >>>> const char *name; >>>> const char *clkname; >>>> struct tidss_vp_feat feat; >>>> } vps[TIDSS_MAX_PORTS]; >>>> >>>> and then use them as: >>>> >>>> .vps = { >>>> { >>>> .name = "kala", >>>> .clkname = "kissa", >>>> .feat.color.has_ctm = true, >>>> }, { >>>> .name = "kala2", >>>> .clkname = "kissa2", >>>> .feat.color.has_ctm = false, >>>> }, >>>> }, >>>> >>>> Perhaps something to try in the future. >>>> >>> >>> Yes, agreed! Having that structure will tidy this up. >>> I will keep this under future work. >>> >>>>> static const u16 tidss_am65x_common_regs[DISPC_COMMON_REG_TABLE_LEN] = { >>>>> @@ -140,8 +142,8 @@ static const u16 tidss_am65x_common_regs[DISPC_COMMON_REG_TABLE_LEN] = { >>>>> const struct dispc_features dispc_am65x_feats = { >>>>> .max_pclk_khz = { >>>>> - [DISPC_VP_DPI] = 165000, >>>>> - [DISPC_VP_OLDI] = 165000, >>>>> + [DISPC_PORT_DPI] = 165000, >>>>> + [DISPC_PORT_OLDI] = 165000, >>>>> }, >>>>> .scaling = { >>>>> @@ -171,7 +173,6 @@ const struct dispc_features dispc_am65x_feats = { >>>>> .vp_name = { "vp1", "vp2" }, >>>>> .ovr_name = { "ovr1", "ovr2" }, >>>>> .vpclk_name = { "vp1", "vp2" }, >>>>> - .vp_bus_type = { DISPC_VP_OLDI, DISPC_VP_DPI }, >>>>> .vp_feat = { .color = { >>>>> .has_ctm = true, >>>>> @@ -185,6 +186,9 @@ const struct dispc_features dispc_am65x_feats = { >>>>> .vid_name = { "vid", "vidl1" }, >>>>> .vid_lite = { false, true, }, >>>>> .vid_order = { 1, 0 }, >>>>> + >>>>> + .num_output_ports = 2, >>>>> + .output_port_bus_type = { DISPC_PORT_OLDI, DISPC_PORT_DPI }, >>>>> }; >>>>> static const u16 tidss_j721e_common_regs[DISPC_COMMON_REG_TABLE_LEN] = { >>>>> @@ -229,8 +233,8 @@ static const u16 tidss_j721e_common_regs[DISPC_COMMON_REG_TABLE_LEN] = { >>>>> const struct dispc_features dispc_j721e_feats = { >>>>> .max_pclk_khz = { >>>>> - [DISPC_VP_DPI] = 170000, >>>>> - [DISPC_VP_INTERNAL] = 600000, >>>>> + [DISPC_PORT_DPI] = 170000, >>>>> + [DISPC_PORT_INTERNAL] = 600000, >>>>> }, >>>>> .scaling = { >>>>> @@ -260,9 +264,7 @@ const struct dispc_features dispc_j721e_feats = { >>>>> .vp_name = { "vp1", "vp2", "vp3", "vp4" }, >>>>> .ovr_name = { "ovr1", "ovr2", "ovr3", "ovr4" }, >>>>> .vpclk_name = { "vp1", "vp2", "vp3", "vp4" }, >>>>> - /* Currently hard coded VP routing (see dispc_initial_config()) */ >>>>> - .vp_bus_type = { DISPC_VP_INTERNAL, DISPC_VP_DPI, >>>>> - DISPC_VP_INTERNAL, DISPC_VP_DPI, }, >>>>> + >>>> >>>> I think this line feed is extra. >>> >>> Okay! Will remove that from all SoC feat structs. >>> >>>> >>>>> .vp_feat = { .color = { >>>>> .has_ctm = true, >>>>> .gamma_size = 1024, >>>>> @@ -273,6 +275,11 @@ const struct dispc_features dispc_j721e_feats = { >>>>> .vid_name = { "vid1", "vidl1", "vid2", "vidl2" }, >>>>> .vid_lite = { 0, 1, 0, 1, }, >>>>> .vid_order = { 1, 3, 0, 2 }, >>>>> + >>>>> + .num_output_ports = 4, >>>>> + /* Currently hard coded VP routing (see dispc_initial_config()) */ >>>>> + .output_port_bus_type = { DISPC_PORT_INTERNAL, DISPC_PORT_DPI, >>>>> + DISPC_PORT_INTERNAL, DISPC_PORT_DPI, }, >>>> >>>> Indent doesn't look right (but it might be just because this is a diff). >>> >>> I may have missed indenting this. >>> >>>> >>>>> }; >>>>> static const u16 *dispc_common_regmap; >>>>> @@ -287,12 +294,12 @@ struct dispc_device { >>>>> void __iomem *base_common; >>>>> void __iomem *base_vid[TIDSS_MAX_PLANES]; >>>>> - void __iomem *base_ovr[TIDSS_MAX_PORTS]; >>>>> - void __iomem *base_vp[TIDSS_MAX_PORTS]; >>>>> + void __iomem *base_ovr[TIDSS_MAX_VPS]; >>>>> + void __iomem *base_vp[TIDSS_MAX_VPS]; >>>>> struct regmap *oldi_io_ctrl; >>>>> - struct clk *vp_clk[TIDSS_MAX_PORTS]; >>>>> + struct clk *vp_clk[TIDSS_MAX_VPS]; >>>>> const struct dispc_features *feat; >>>>> @@ -300,7 +307,7 @@ struct dispc_device { >>>>> bool is_enabled; >>>>> - struct dss_vp_data vp_data[TIDSS_MAX_PORTS]; >>>>> + struct dss_vp_data vp_data[TIDSS_MAX_VPS]; >>>>> u32 *fourccs; >>>>> u32 num_fourccs; >>>>> @@ -851,7 +858,7 @@ int dispc_vp_bus_check(struct dispc_device *dispc, u32 hw_videoport, >>>>> return -EINVAL; >>>>> } >>>>> - if (dispc->feat->vp_bus_type[hw_videoport] != DISPC_VP_OLDI && >>>>> + if (dispc->feat->output_port_bus_type[hw_videoport] != DISPC_PORT_OLDI && >>>> >>>> Hmm, so is the hw_videoport a vp index or an output index? Sounds like >>>> the former, so it's not right, even if at the moment they're identical. >>>> We need some kind of mapping between those. >>>> >>> >>> It is indeed a vp index. And yes, I can come up with a mapping mechanism. >>> >>>> If the mapping can be changed (or just defined in the DT), I think we >>>> need a variable in struct dispc_device, which tells the output to which >>>> a videoport is connected to. Or vice versa, I'm not sure which direction >>>> we need more. If the mapping is always the same on all SoC (but I don't >>>> think so), we can have it in the feats. >>>> >>> >>> As of now, the mapping is always same. But I would like to make is >>> generalized for future. Hence, I am considering to keep the variable in >>> struct dispc_device. >>> >>> My question though would be, how would one be able to find which kind >>> of device is the port connected to, if it is connected to a bridge? For >>> example, in case of panels, we have a "connector_type" variable in >>> drm_panel which tells what kind of sink it is. But there is no such >>> thing in drm_bridge. >>> >>> This is required because what if we can connect an videoport to either >>> an LVDS/OLDI bridge or a DPI bridge. >> >> The connector type shouldn't matter. >> >> The DSS has VPs and outputs. The VPs are "generic" and identical to each >> other, except in their possible connections to the outputs. The outputs, >> at least at the moment, are DPI, LVDS and internal, where internal is >> basically just DPI. >> >> Those are the three different cases we are interested in within the dss >> driver, right? Does it matter where the DPI or LVDS output goes? >> > > I believe it does. =) > > While the VPs do always transmit DPI signals, the code in tidss_dispc.c > uses the information of the bus connected at the endpoint to configure > the OLDI parameters, and to turn OLDI IOs on and off in > dispc_vp_(prepare/unprepare). > > Up until now, the outputs have been fixed (VP0 -> OLDI, VP1 -> DPI), and > the code used the enum dispc_vp_bus_type to differntiate between LVDS or > DPI requirements. But for a general case where output from VP0 could > either use the OLDI TXes and send out LVDS signals OR bypass the OLDI > TXes and send out DPI signals directly, we would need a mechanism to > find out which sink is present at the end, LVDS or DPI. > > I assumed, with that mechanism, we could (re)configure the vp-to-output > mapping, which then would be used in the various places in > tidss_dispc.c. But we should already know all that. Say, on AM625, we have three ports (or "outputs") (defined in DT), OLDI1, DPI, OLDI2. If there's an endpoint configured for the first port, we know we need to set up OLDI, and we need a VP for it. If the hardware mapping between VPs and outputs is hardcoded, we know directly that VP0 is needed, and it's used for OLDI. So now we have the mapping of VP0 -> OLDI (port1). If (say) VP0 could alternatively be used for DPI output, then we'd see the second port, DPI, having an endpoint configured. Having both OLDI1 and DPI endpoints would be invalid, of course. So "how would one be able to find which kind of device is the port connected to" is irrelevant, I think. We know the output port, so we know the bus type, and we don't really need to know anything else about what's there behind the bus. Or is there some detail I'm missing here? Tomi
diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c index 165365b515e1..c1c4faccbddc 100644 --- a/drivers/gpu/drm/tidss/tidss_dispc.c +++ b/drivers/gpu/drm/tidss/tidss_dispc.c @@ -61,7 +61,7 @@ const struct dispc_features dispc_k2g_feats = { .min_pclk_khz = 4375, .max_pclk_khz = { - [DISPC_VP_DPI] = 150000, + [DISPC_PORT_DPI] = 150000, }, /* @@ -96,7 +96,6 @@ const struct dispc_features dispc_k2g_feats = { .vp_name = { "vp1" }, .ovr_name = { "ovr1" }, .vpclk_name = { "vp1" }, - .vp_bus_type = { DISPC_VP_DPI }, .vp_feat = { .color = { .has_ctm = true, @@ -109,6 +108,9 @@ const struct dispc_features dispc_k2g_feats = { .vid_name = { "vid1" }, .vid_lite = { false }, .vid_order = { 0 }, + + .num_output_ports = 1, + .output_port_bus_type = { DISPC_PORT_DPI }, }; static const u16 tidss_am65x_common_regs[DISPC_COMMON_REG_TABLE_LEN] = { @@ -140,8 +142,8 @@ static const u16 tidss_am65x_common_regs[DISPC_COMMON_REG_TABLE_LEN] = { const struct dispc_features dispc_am65x_feats = { .max_pclk_khz = { - [DISPC_VP_DPI] = 165000, - [DISPC_VP_OLDI] = 165000, + [DISPC_PORT_DPI] = 165000, + [DISPC_PORT_OLDI] = 165000, }, .scaling = { @@ -171,7 +173,6 @@ const struct dispc_features dispc_am65x_feats = { .vp_name = { "vp1", "vp2" }, .ovr_name = { "ovr1", "ovr2" }, .vpclk_name = { "vp1", "vp2" }, - .vp_bus_type = { DISPC_VP_OLDI, DISPC_VP_DPI }, .vp_feat = { .color = { .has_ctm = true, @@ -185,6 +186,9 @@ const struct dispc_features dispc_am65x_feats = { .vid_name = { "vid", "vidl1" }, .vid_lite = { false, true, }, .vid_order = { 1, 0 }, + + .num_output_ports = 2, + .output_port_bus_type = { DISPC_PORT_OLDI, DISPC_PORT_DPI }, }; static const u16 tidss_j721e_common_regs[DISPC_COMMON_REG_TABLE_LEN] = { @@ -229,8 +233,8 @@ static const u16 tidss_j721e_common_regs[DISPC_COMMON_REG_TABLE_LEN] = { const struct dispc_features dispc_j721e_feats = { .max_pclk_khz = { - [DISPC_VP_DPI] = 170000, - [DISPC_VP_INTERNAL] = 600000, + [DISPC_PORT_DPI] = 170000, + [DISPC_PORT_INTERNAL] = 600000, }, .scaling = { @@ -260,9 +264,7 @@ const struct dispc_features dispc_j721e_feats = { .vp_name = { "vp1", "vp2", "vp3", "vp4" }, .ovr_name = { "ovr1", "ovr2", "ovr3", "ovr4" }, .vpclk_name = { "vp1", "vp2", "vp3", "vp4" }, - /* Currently hard coded VP routing (see dispc_initial_config()) */ - .vp_bus_type = { DISPC_VP_INTERNAL, DISPC_VP_DPI, - DISPC_VP_INTERNAL, DISPC_VP_DPI, }, + .vp_feat = { .color = { .has_ctm = true, .gamma_size = 1024, @@ -273,6 +275,11 @@ const struct dispc_features dispc_j721e_feats = { .vid_name = { "vid1", "vidl1", "vid2", "vidl2" }, .vid_lite = { 0, 1, 0, 1, }, .vid_order = { 1, 3, 0, 2 }, + + .num_output_ports = 4, + /* Currently hard coded VP routing (see dispc_initial_config()) */ + .output_port_bus_type = { DISPC_PORT_INTERNAL, DISPC_PORT_DPI, + DISPC_PORT_INTERNAL, DISPC_PORT_DPI, }, }; static const u16 *dispc_common_regmap; @@ -287,12 +294,12 @@ struct dispc_device { void __iomem *base_common; void __iomem *base_vid[TIDSS_MAX_PLANES]; - void __iomem *base_ovr[TIDSS_MAX_PORTS]; - void __iomem *base_vp[TIDSS_MAX_PORTS]; + void __iomem *base_ovr[TIDSS_MAX_VPS]; + void __iomem *base_vp[TIDSS_MAX_VPS]; struct regmap *oldi_io_ctrl; - struct clk *vp_clk[TIDSS_MAX_PORTS]; + struct clk *vp_clk[TIDSS_MAX_VPS]; const struct dispc_features *feat; @@ -300,7 +307,7 @@ struct dispc_device { bool is_enabled; - struct dss_vp_data vp_data[TIDSS_MAX_PORTS]; + struct dss_vp_data vp_data[TIDSS_MAX_VPS]; u32 *fourccs; u32 num_fourccs; @@ -851,7 +858,7 @@ int dispc_vp_bus_check(struct dispc_device *dispc, u32 hw_videoport, return -EINVAL; } - if (dispc->feat->vp_bus_type[hw_videoport] != DISPC_VP_OLDI && + if (dispc->feat->output_port_bus_type[hw_videoport] != DISPC_PORT_OLDI && fmt->is_oldi_fmt) { dev_dbg(dispc->dev, "%s: %s is not OLDI-port\n", __func__, dispc->feat->vp_name[hw_videoport]); @@ -955,7 +962,7 @@ void dispc_vp_prepare(struct dispc_device *dispc, u32 hw_videoport, if (WARN_ON(!fmt)) return; - if (dispc->feat->vp_bus_type[hw_videoport] == DISPC_VP_OLDI) { + if (dispc->feat->output_port_bus_type[hw_videoport] == DISPC_PORT_OLDI) { dispc_oldi_tx_power(dispc, true); dispc_enable_oldi(dispc, hw_videoport, fmt); @@ -1014,7 +1021,7 @@ void dispc_vp_enable(struct dispc_device *dispc, u32 hw_videoport, align = true; /* always use DE_HIGH for OLDI */ - if (dispc->feat->vp_bus_type[hw_videoport] == DISPC_VP_OLDI) + if (dispc->feat->output_port_bus_type[hw_videoport] == DISPC_PORT_OLDI) ieo = false; dispc_vp_write(dispc, hw_videoport, DISPC_VP_POL_FREQ, @@ -1040,7 +1047,7 @@ void dispc_vp_disable(struct dispc_device *dispc, u32 hw_videoport) void dispc_vp_unprepare(struct dispc_device *dispc, u32 hw_videoport) { - if (dispc->feat->vp_bus_type[hw_videoport] == DISPC_VP_OLDI) { + if (dispc->feat->output_port_bus_type[hw_videoport] == DISPC_PORT_OLDI) { dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, 0); dispc_oldi_tx_power(dispc, false); @@ -1116,10 +1123,10 @@ enum drm_mode_status dispc_vp_mode_valid(struct dispc_device *dispc, const struct drm_display_mode *mode) { u32 hsw, hfp, hbp, vsw, vfp, vbp; - enum dispc_vp_bus_type bus_type; + enum dispc_port_bus_type bus_type; int max_pclk; - bus_type = dispc->feat->vp_bus_type[hw_videoport]; + bus_type = dispc->feat->output_port_bus_type[hw_videoport]; max_pclk = dispc->feat->max_pclk_khz[bus_type]; diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h b/drivers/gpu/drm/tidss/tidss_dispc.h index e49432f0abf5..30fb44158347 100644 --- a/drivers/gpu/drm/tidss/tidss_dispc.h +++ b/drivers/gpu/drm/tidss/tidss_dispc.h @@ -50,11 +50,11 @@ struct dispc_errata { bool i2000; /* DSS Does Not Support YUV Pixel Data Formats */ }; -enum dispc_vp_bus_type { - DISPC_VP_DPI, /* DPI output */ - DISPC_VP_OLDI, /* OLDI (LVDS) output */ - DISPC_VP_INTERNAL, /* SoC internal routing */ - DISPC_VP_MAX_BUS_TYPE, +enum dispc_port_bus_type { + DISPC_PORT_DPI, /* DPI output */ + DISPC_PORT_OLDI, /* OLDI (LVDS) output */ + DISPC_PORT_INTERNAL, /* SoC internal routing */ + DISPC_PORT_MAX_BUS_TYPE, }; enum dispc_dss_subrevision { @@ -65,7 +65,7 @@ enum dispc_dss_subrevision { struct dispc_features { int min_pclk_khz; - int max_pclk_khz[DISPC_VP_MAX_BUS_TYPE]; + int max_pclk_khz[DISPC_PORT_MAX_BUS_TYPE]; struct dispc_features_scaling scaling; @@ -74,15 +74,16 @@ struct dispc_features { const char *common; const u16 *common_regs; u32 num_vps; - const char *vp_name[TIDSS_MAX_PORTS]; /* Should match dt reg names */ - const char *ovr_name[TIDSS_MAX_PORTS]; /* Should match dt reg names */ - const char *vpclk_name[TIDSS_MAX_PORTS]; /* Should match dt clk names */ - const enum dispc_vp_bus_type vp_bus_type[TIDSS_MAX_PORTS]; + const char *vp_name[TIDSS_MAX_VPS]; /* Should match dt reg names */ + const char *ovr_name[TIDSS_MAX_VPS]; /* Should match dt reg names */ + const char *vpclk_name[TIDSS_MAX_VPS]; /* Should match dt clk names */ struct tidss_vp_feat vp_feat; u32 num_planes; const char *vid_name[TIDSS_MAX_PLANES]; /* Should match dt reg names */ bool vid_lite[TIDSS_MAX_PLANES]; u32 vid_order[TIDSS_MAX_PLANES]; + u32 num_output_ports; + const enum dispc_port_bus_type output_port_bus_type[TIDSS_MAX_OUTPUT_PORTS]; }; extern const struct dispc_features dispc_k2g_feats; diff --git a/drivers/gpu/drm/tidss/tidss_drv.h b/drivers/gpu/drm/tidss/tidss_drv.h index d7f27b0b0315..0ce7ee5ccd5b 100644 --- a/drivers/gpu/drm/tidss/tidss_drv.h +++ b/drivers/gpu/drm/tidss/tidss_drv.h @@ -9,8 +9,9 @@ #include <linux/spinlock.h> -#define TIDSS_MAX_PORTS 4 +#define TIDSS_MAX_VPS 4 #define TIDSS_MAX_PLANES 4 +#define TIDSS_MAX_OUTPUT_PORTS 4 typedef u32 dispc_irq_t; @@ -22,7 +23,7 @@ struct tidss_device { struct dispc_device *dispc; unsigned int num_crtcs; - struct drm_crtc *crtcs[TIDSS_MAX_PORTS]; + struct drm_crtc *crtcs[TIDSS_MAX_VPS]; unsigned int num_planes; struct drm_plane *planes[TIDSS_MAX_PLANES]; diff --git a/drivers/gpu/drm/tidss/tidss_irq.h b/drivers/gpu/drm/tidss/tidss_irq.h index b512614d5863..a753f5e3ce15 100644 --- a/drivers/gpu/drm/tidss/tidss_irq.h +++ b/drivers/gpu/drm/tidss/tidss_irq.h @@ -35,7 +35,7 @@ #define DSS_IRQ_VP_BIT_N(ch, bit) (4 + 4 * (ch) + (bit)) #define DSS_IRQ_PLANE_BIT_N(plane, bit) \ - (DSS_IRQ_VP_BIT_N(TIDSS_MAX_PORTS, 0) + 1 * (plane) + (bit)) + (DSS_IRQ_VP_BIT_N(TIDSS_MAX_VPS, 0) + 1 * (plane) + (bit)) #define DSS_IRQ_VP_BIT(ch, bit) BIT(DSS_IRQ_VP_BIT_N((ch), (bit))) #define DSS_IRQ_PLANE_BIT(plane, bit) \ diff --git a/drivers/gpu/drm/tidss/tidss_kms.c b/drivers/gpu/drm/tidss/tidss_kms.c index ad2fa3c3d4a7..d449131935d2 100644 --- a/drivers/gpu/drm/tidss/tidss_kms.c +++ b/drivers/gpu/drm/tidss/tidss_kms.c @@ -118,16 +118,16 @@ static int tidss_dispc_modeset_init(struct tidss_device *tidss) }; const struct dispc_features *feat = tidss->feat; - u32 max_vps = feat->num_vps; + u32 output_ports = feat->num_output_ports; u32 max_planes = feat->num_planes; - struct pipe pipes[TIDSS_MAX_PORTS]; + struct pipe pipes[TIDSS_MAX_VPS]; u32 num_pipes = 0; u32 crtc_mask; /* first find all the connected panels & bridges */ - for (i = 0; i < max_vps; i++) { + for (i = 0; i < output_ports; i++) { struct drm_panel *panel; struct drm_bridge *bridge; u32 enc_type = DRM_MODE_ENCODER_NONE; @@ -148,12 +148,12 @@ static int tidss_dispc_modeset_init(struct tidss_device *tidss) dev_dbg(dev, "Setting up panel for port %d\n", i); - switch (feat->vp_bus_type[i]) { - case DISPC_VP_OLDI: + switch (feat->output_port_bus_type[i]) { + case DISPC_PORT_OLDI: enc_type = DRM_MODE_ENCODER_LVDS; conn_type = DRM_MODE_CONNECTOR_LVDS; break; - case DISPC_VP_DPI: + case DISPC_PORT_DPI: enc_type = DRM_MODE_ENCODER_DPI; conn_type = DRM_MODE_CONNECTOR_DPI; break;
Make DSS Video Ports agnostic of output bus types. DSS controllers have had a 1-to-1 coupling between its VPs and its output ports. This no longer stands true for the new AM625 DSS. This coupling, hence, has been removed by renaming the 'vp_bus_type' to 'output_port_bus_type' because the VPs are essentially agnostic of the bus type and it is the output ports which have a bus type. The AM625 DSS has 2 VPs but requires 3 output ports to support its Dual-Link OLDI video output coming from a single VP. Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com> --- drivers/gpu/drm/tidss/tidss_dispc.c | 47 +++++++++++++++++------------ drivers/gpu/drm/tidss/tidss_dispc.h | 21 +++++++------ drivers/gpu/drm/tidss/tidss_drv.h | 5 +-- drivers/gpu/drm/tidss/tidss_irq.h | 2 +- drivers/gpu/drm/tidss/tidss_kms.c | 12 ++++---- 5 files changed, 48 insertions(+), 39 deletions(-)