| Message ID | 1382358067-5578-2-git-send-email-shobhit.kumar@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Shobhit - On Mon, 21 Oct 2013, Shobhit Kumar <shobhit.kumar@intel.com> wrote: > Also add new fields in intel_dsi to have all dphy related parameters. > These will be useful even when we go for pure generic MIPI design I feel like we have a different idea of what the ideal generic design is. For me, the goal is that we change the struct intel_dsi_device to struct drm_bridge, and those drm_bridge drivers are all about the panel, and have as few details about i915 or our hardware as possible. Having the bridge drivers fill in register values to be written by the core DSI code does not fit that. Yes, I had some of those in my bridge conversion patches too, but I did not intend we'd keep adding more. I'd rather we provide generic helpers the bridge driver can call. > Yogesh Mohan Marimuthu <yogesh.mohan.marimuthu@intel.com> > Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com> > --- > drivers/gpu/drm/i915/intel_dsi.c | 9 ++++++++- > drivers/gpu/drm/i915/intel_dsi.h | 29 +++++++++++++++++++++++++++++ > 2 files changed, 37 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c > index 9a2fdd2..34e19b7 100644 > --- a/drivers/gpu/drm/i915/intel_dsi.c > +++ b/drivers/gpu/drm/i915/intel_dsi.c > @@ -147,6 +147,9 @@ static void intel_dsi_enable(struct intel_encoder *encoder) > > DRM_DEBUG_KMS("\n"); > > + if (intel_dsi->dev.dev_ops->panel_reset) > + intel_dsi->dev.dev_ops->panel_reset(&intel_dsi->dev); Would this map to ->pre_enable in drm_bridge? > + > temp = I915_READ(MIPI_DEVICE_READY(pipe)); > if ((temp & DEVICE_READY) == 0) { > temp &= ~ULPS_STATE_MASK; > @@ -162,6 +165,9 @@ static void intel_dsi_enable(struct intel_encoder *encoder) > I915_WRITE(MIPI_DEVICE_READY(pipe), temp); > } > > + if (intel_dsi->dev.dev_ops->send_otp_cmds) > + intel_dsi->dev.dev_ops->send_otp_cmds(&intel_dsi->dev); What is otp? one time programming? Why not in ->enable? > + > if (is_cmd_mode(intel_dsi)) > I915_WRITE(MIPI_MAX_RETURN_PKT_SIZE(pipe), 8 * 4); > > @@ -176,7 +182,8 @@ static void intel_dsi_enable(struct intel_encoder *encoder) > POSTING_READ(MIPI_PORT_CTRL(pipe)); > } > > - intel_dsi->dev.dev_ops->enable(&intel_dsi->dev); > + if (intel_dsi->dev.dev_ops->enable) > + intel_dsi->dev.dev_ops->enable(&intel_dsi->dev); > } > > static void intel_dsi_disable(struct intel_encoder *encoder) > diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h > index c7765f3..b71c9b3 100644 > --- a/drivers/gpu/drm/i915/intel_dsi.h > +++ b/drivers/gpu/drm/i915/intel_dsi.h > @@ -39,6 +39,13 @@ struct intel_dsi_device { > struct intel_dsi_dev_ops { > bool (*init)(struct intel_dsi_device *dsi); > > + void (*panel_reset)(struct intel_dsi_device *dsi); > + > + void (*disable_panel_power)(struct intel_dsi_device *dsi); What is the enabling counterpart to disable_panel_power? panel_reset? > + > + /* send one time programmable commands */ > + void (*send_otp_cmds)(struct intel_dsi_device *dsi); > + > /* This callback must be able to assume DSI commands can be sent */ > void (*enable)(struct intel_dsi_device *dsi); > > @@ -89,6 +96,28 @@ struct intel_dsi { > > /* eot for MIPI_EOT_DISABLE register */ > u32 eot_disable; > + > + u16 dsi_clock_freq; > + u8 video_mode_type; > + u32 data_width; > + u8 dither; > + u32 port_bits; > + u8 escape_clk_div; > + u32 lp_rx_timeout; > + u8 turn_arnd_val; > + u16 init_count; > + u16 rst_timer_val; > + u16 hs_to_lp_count; > + u16 lp_byte_clk; > + u32 bw_timer; > + u16 clk_lp_to_hs_count; > + u16 clk_hs_to_lp_count; > + u32 video_frmt_cfg_bits; > + u32 dphy_reg; > + > + u8 backlight_off_delay; /*in ms*/ > + bool send_shutdown; > + u8 shutdown_pkt_delay; /*in ms*/ > }; > > static inline struct intel_dsi *enc_to_intel_dsi(struct drm_encoder *encoder) > -- > 1.7.9.5 >
On 10/21/2013 6:57 PM, Jani Nikula wrote: > > Hi Shobhit - > > On Mon, 21 Oct 2013, Shobhit Kumar <shobhit.kumar@intel.com> wrote: >> Also add new fields in intel_dsi to have all dphy related parameters. >> These will be useful even when we go for pure generic MIPI design > > I feel like we have a different idea of what the ideal generic design > is. For me, the goal is that we change the struct intel_dsi_device to > struct drm_bridge, and those drm_bridge drivers are all about the panel, > and have as few details about i915 or our hardware as possible. Having > the bridge drivers fill in register values to be written by the core DSI > code does not fit that. Yes, I had some of those in my bridge conversion > patches too, but I did not intend we'd keep adding more. > > I'd rather we provide generic helpers the bridge driver can call. Yeah, look like our ideas are different. In your goal with drm_bridge architecture, we will still end up having multiple bridge drivers for each different panel. But my goal is to have a single driver which can work for multiple panels. Since we already have enabled some panels with sub-encoder architecture for completeness I was planning to maintain generic driver as one sub-encoder. But actually we can do away with all sub-encoder and do not need even drm_bridge and all implementation will be in intel_dsi.c. Panel specific configurations or sequences will come from VBT which I have tried to convert as parameters. All the parameters DSI/DPHY spec specific and none of them particularly relate to our hardware. > >> Yogesh Mohan Marimuthu <yogesh.mohan.marimuthu@intel.com> >> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com> >> --- >> drivers/gpu/drm/i915/intel_dsi.c | 9 ++++++++- >> drivers/gpu/drm/i915/intel_dsi.h | 29 +++++++++++++++++++++++++++++ >> 2 files changed, 37 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c >> index 9a2fdd2..34e19b7 100644 >> --- a/drivers/gpu/drm/i915/intel_dsi.c >> +++ b/drivers/gpu/drm/i915/intel_dsi.c >> @@ -147,6 +147,9 @@ static void intel_dsi_enable(struct intel_encoder *encoder) >> >> DRM_DEBUG_KMS("\n"); >> >> + if (intel_dsi->dev.dev_ops->panel_reset) >> + intel_dsi->dev.dev_ops->panel_reset(&intel_dsi->dev); > > Would this map to ->pre_enable in drm_bridge? I have not yet migrated to drm_bridge and need to check thes call flows for drm_bridge > >> + >> temp = I915_READ(MIPI_DEVICE_READY(pipe)); >> if ((temp & DEVICE_READY) == 0) { >> temp &= ~ULPS_STATE_MASK; >> @@ -162,6 +165,9 @@ static void intel_dsi_enable(struct intel_encoder *encoder) >> I915_WRITE(MIPI_DEVICE_READY(pipe), temp); >> } >> >> + if (intel_dsi->dev.dev_ops->send_otp_cmds) >> + intel_dsi->dev.dev_ops->send_otp_cmds(&intel_dsi->dev); > > What is otp? one time programming? Why not in ->enable? Yes. OTP is done before sending pixel stream and enable is done after we start pixel stream > >> + >> if (is_cmd_mode(intel_dsi)) >> I915_WRITE(MIPI_MAX_RETURN_PKT_SIZE(pipe), 8 * 4); >> >> @@ -176,7 +182,8 @@ static void intel_dsi_enable(struct intel_encoder *encoder) >> POSTING_READ(MIPI_PORT_CTRL(pipe)); >> } >> >> - intel_dsi->dev.dev_ops->enable(&intel_dsi->dev); >> + if (intel_dsi->dev.dev_ops->enable) >> + intel_dsi->dev.dev_ops->enable(&intel_dsi->dev); >> } >> >> static void intel_dsi_disable(struct intel_encoder *encoder) >> diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h >> index c7765f3..b71c9b3 100644 >> --- a/drivers/gpu/drm/i915/intel_dsi.h >> +++ b/drivers/gpu/drm/i915/intel_dsi.h >> @@ -39,6 +39,13 @@ struct intel_dsi_device { >> struct intel_dsi_dev_ops { >> bool (*init)(struct intel_dsi_device *dsi); >> >> + void (*panel_reset)(struct intel_dsi_device *dsi); >> + >> + void (*disable_panel_power)(struct intel_dsi_device *dsi); > > What is the enabling counterpart to disable_panel_power? panel_reset? Yes. > >> + >> + /* send one time programmable commands */ >> + void (*send_otp_cmds)(struct intel_dsi_device *dsi); >> + >> /* This callback must be able to assume DSI commands can be sent */ >> void (*enable)(struct intel_dsi_device *dsi); >> >> @@ -89,6 +96,28 @@ struct intel_dsi { >> >> /* eot for MIPI_EOT_DISABLE register */ >> u32 eot_disable; >> + >> + u16 dsi_clock_freq; >> + u8 video_mode_type; >> + u32 data_width; >> + u8 dither; >> + u32 port_bits; >> + u8 escape_clk_div; >> + u32 lp_rx_timeout; >> + u8 turn_arnd_val; >> + u16 init_count; >> + u16 rst_timer_val; >> + u16 hs_to_lp_count; >> + u16 lp_byte_clk; >> + u32 bw_timer; >> + u16 clk_lp_to_hs_count; >> + u16 clk_hs_to_lp_count; >> + u32 video_frmt_cfg_bits; >> + u32 dphy_reg; >> + >> + u8 backlight_off_delay; /*in ms*/ >> + bool send_shutdown; >> + u8 shutdown_pkt_delay; /*in ms*/ >> }; >> >> static inline struct intel_dsi *enc_to_intel_dsi(struct drm_encoder *encoder) >> -- >> 1.7.9.5 >> > Regards Shobhit
On Tue, 22 Oct 2013, Shobhit Kumar <shobhit.kumar@intel.com> wrote: > On 10/21/2013 6:57 PM, Jani Nikula wrote: >> >> Hi Shobhit - >> >> On Mon, 21 Oct 2013, Shobhit Kumar <shobhit.kumar@intel.com> wrote: >>> Also add new fields in intel_dsi to have all dphy related parameters. >>> These will be useful even when we go for pure generic MIPI design >> >> I feel like we have a different idea of what the ideal generic design >> is. For me, the goal is that we change the struct intel_dsi_device to >> struct drm_bridge, and those drm_bridge drivers are all about the panel, >> and have as few details about i915 or our hardware as possible. Having >> the bridge drivers fill in register values to be written by the core DSI >> code does not fit that. Yes, I had some of those in my bridge conversion >> patches too, but I did not intend we'd keep adding more. >> >> I'd rather we provide generic helpers the bridge driver can call. > > Yeah, look like our ideas are different. In your goal with drm_bridge > architecture, we will still end up having multiple bridge drivers for > each different panel. But my goal is to have a single driver which can > work for multiple panels. I'm trying to look one or two steps further, and what it will mean to the driver. Here's the long term goal in upstream as I see it: There will be a framework in place that allows one to write a (DSI) panel driver once, using generic APIs, and use that panel driver with any SoC (that implements the other side of the framework). We are obviously far away from that goal at the moment. But IMHO we should keep that in mind as a guide to what we are doing now. Moving towards a model with a clearly defined API between the DSI core and the panels, where the panel specific things are abstracted away from the core, or towards a model where the core and panel driver depend on the implementation of each other, communicating via variables. > Since we already have enabled some panels with sub-encoder > architecture for completeness I was planning to maintain generic > driver as one sub-encoder. Nothing prevents you from doing that, as long as the separation between the core and the panel drivers remains clear. > But actually we can do away with all sub-encoder and do not need even > drm_bridge and all implementation will be in intel_dsi.c. Panel > specific configurations or sequences will come from VBT which I have > tried to convert as parameters. With this model it is all too easy to forget what is the panel driver and what is the SoC driver. They *are* two separate things, and should not be mixed. It will be all too easy to keep adding new parameters and conditions in the code as new panel drivers appear to need them. It will lead to code that is very difficult to understand and maintain. A model similar to what I'm proposing has also been tried and tested, with several panels: drivers/video/omap2/. It's not DRM, and the control is in the panel drivers, but the separation is extremely clear (panel drivers are separate kernel modules). No doubt the clear separation between the core and the panel drivers will be harder and more work in the short term, but it will pay off in the long term. And it doesn't all have to happen at once, as long as we work *towards* that goal, not away from it. BR, Jani.
Hi Jani, On 10/22/2013 05:23 PM, Jani Nikula wrote: > On Tue, 22 Oct 2013, Shobhit Kumar <shobhit.kumar@intel.com> wrote: >> On 10/21/2013 6:57 PM, Jani Nikula wrote: >>> >>> Hi Shobhit - >>> >>> On Mon, 21 Oct 2013, Shobhit Kumar <shobhit.kumar@intel.com> wrote: >>>> Also add new fields in intel_dsi to have all dphy related parameters. >>>> These will be useful even when we go for pure generic MIPI design >>> >>> I feel like we have a different idea of what the ideal generic design >>> is. For me, the goal is that we change the struct intel_dsi_device to >>> struct drm_bridge, and those drm_bridge drivers are all about the panel, >>> and have as few details about i915 or our hardware as possible. Having >>> the bridge drivers fill in register values to be written by the core DSI >>> code does not fit that. Yes, I had some of those in my bridge conversion >>> patches too, but I did not intend we'd keep adding more. >>> >>> I'd rather we provide generic helpers the bridge driver can call. >> >> Yeah, look like our ideas are different. In your goal with drm_bridge >> architecture, we will still end up having multiple bridge drivers for >> each different panel. But my goal is to have a single driver which can >> work for multiple panels. > > I'm trying to look one or two steps further, and what it will mean to > the driver. Here's the long term goal in upstream as I see it: There > will be a framework in place that allows one to write a (DSI) panel > driver once, using generic APIs, and use that panel driver with any SoC > (that implements the other side of the framework). To clarify in terms of what we have currently, in my opinion intel_dsi.c is one such SoC specific implementation for BYT and associated MIPI host controller. And sub-encoder drivers are other end and I want to unify sub-encoder side to have just one sub-encoder for any panel out there. What you are talking makes perfect sense if we are going to have each panel driver as a separate driver. But we can still achieve this separation with common panel driver as well. > > We are obviously far away from that goal at the moment. But IMHO we > should keep that in mind as a guide to what we are doing now. Moving > towards a model with a clearly defined API between the DSI core and the > panels, where the panel specific things are abstracted away from the > core, or towards a model where the core and panel driver depend on the > implementation of each other, communicating via variables. I think we are aligned on the goal and I feel there is a need for common DSI core to be separate > >> Since we already have enabled some panels with sub-encoder >> architecture for completeness I was planning to maintain generic >> driver as one sub-encoder. > > Nothing prevents you from doing that, as long as the separation between > the core and the panel drivers remains clear. > >> But actually we can do away with all sub-encoder and do not need even >> drm_bridge and all implementation will be in intel_dsi.c. Panel >> specific configurations or sequences will come from VBT which I have >> tried to convert as parameters. > > With this model it is all too easy to forget what is the panel driver > and what is the SoC driver. They *are* two separate things, and should > not be mixed. It will be all too easy to keep adding new parameters and > conditions in the code as new panel drivers appear to need them. It will > lead to code that is very difficult to understand and maintain. I think here you have misunderstood my proposal. I still treat SoC driver and actual Panel driver as separate. And whatever parameters I have tried to add are all DSI/DPHY spec related. There is not even single parameter which is panel specific. If you are confusing this because of use of - if (intel_dsi->dsi_clock_freq) dsi_clk = intel_dsi->dsi_clock_freq; I feel it is okay to have this parameter which provides provision for spec parameters to be hard-coded instead of calculating if a panel needs and this is *only* such parameter and I already have ways to remove it as well. IMHO it is a small price to pay to get one generic panel driver. That is another thing if the DSI/DPHy specs themselves evolve and we need to modify our core, but that is unavoidable I guess. > > A model similar to what I'm proposing has also been tried and tested, > with several panels: drivers/video/omap2/. It's not DRM, and the control > is in the panel drivers, but the separation is extremely clear (panel > drivers are separate kernel modules). Understand, but again my idea is to have one single driver which can work for all panels and hence there is no need of multiple panel drivers. This works with SoC side of the framework. I have not yet described how we can achieve this, but first does it has any merit to have something like this ? > > No doubt the clear separation between the core and the panel drivers > will be harder and more work in the short term, but it will pay off in > the long term. And it doesn't all have to happen at once, as long as we > work *towards* that goal, not away from it. I think we should take into account the amount of effort required to develop and maintain bridge drivers for tens of MIPI panel out there Vs having one panel driver to maintain and make fully spec compliant taking care of open ends left by the specs in the best way we can to achieve this generality. Regards Shobhit
On Wed, 23 Oct 2013, Shobhit Kumar <shobhit.kumar@intel.com> wrote: > Hi Jani, > On 10/22/2013 05:23 PM, Jani Nikula wrote: >> On Tue, 22 Oct 2013, Shobhit Kumar <shobhit.kumar@intel.com> wrote: >>> On 10/21/2013 6:57 PM, Jani Nikula wrote: >>>> >>>> Hi Shobhit - >>>> >>>> On Mon, 21 Oct 2013, Shobhit Kumar <shobhit.kumar@intel.com> wrote: >>>>> Also add new fields in intel_dsi to have all dphy related parameters. >>>>> These will be useful even when we go for pure generic MIPI design >>>> >>>> I feel like we have a different idea of what the ideal generic design >>>> is. For me, the goal is that we change the struct intel_dsi_device to >>>> struct drm_bridge, and those drm_bridge drivers are all about the panel, >>>> and have as few details about i915 or our hardware as possible. Having >>>> the bridge drivers fill in register values to be written by the core DSI >>>> code does not fit that. Yes, I had some of those in my bridge conversion >>>> patches too, but I did not intend we'd keep adding more. >>>> >>>> I'd rather we provide generic helpers the bridge driver can call. >>> >>> Yeah, look like our ideas are different. In your goal with drm_bridge >>> architecture, we will still end up having multiple bridge drivers for >>> each different panel. But my goal is to have a single driver which can >>> work for multiple panels. >> >> I'm trying to look one or two steps further, and what it will mean to >> the driver. Here's the long term goal in upstream as I see it: There >> will be a framework in place that allows one to write a (DSI) panel >> driver once, using generic APIs, and use that panel driver with any SoC >> (that implements the other side of the framework). > > To clarify in terms of what we have currently, in my opinion intel_dsi.c > is one such SoC specific implementation for BYT and associated MIPI host > controller. And sub-encoder drivers are other end and I want to unify > sub-encoder side to have just one sub-encoder for any panel out there. > What you are talking makes perfect sense if we are going to have each > panel driver as a separate driver. But we can still achieve this > separation with common panel driver as well. > >> >> We are obviously far away from that goal at the moment. But IMHO we >> should keep that in mind as a guide to what we are doing now. Moving >> towards a model with a clearly defined API between the DSI core and the >> panels, where the panel specific things are abstracted away from the >> core, or towards a model where the core and panel driver depend on the >> implementation of each other, communicating via variables. > > I think we are aligned on the goal and I feel there is a need for common > DSI core to be separate > >> >>> Since we already have enabled some panels with sub-encoder >>> architecture for completeness I was planning to maintain generic >>> driver as one sub-encoder. >> >> Nothing prevents you from doing that, as long as the separation between >> the core and the panel drivers remains clear. >> >>> But actually we can do away with all sub-encoder and do not need even >>> drm_bridge and all implementation will be in intel_dsi.c. Panel >>> specific configurations or sequences will come from VBT which I have >>> tried to convert as parameters. >> >> With this model it is all too easy to forget what is the panel driver >> and what is the SoC driver. They *are* two separate things, and should >> not be mixed. It will be all too easy to keep adding new parameters and >> conditions in the code as new panel drivers appear to need them. It will >> lead to code that is very difficult to understand and maintain. > > I think here you have misunderstood my proposal. I still treat SoC > driver and actual Panel driver as separate. And whatever parameters I > have tried to add are all DSI/DPHY spec related. There is not even > single parameter which is panel specific. If you are confusing this > because of use of - > > if (intel_dsi->dsi_clock_freq) > dsi_clk = intel_dsi->dsi_clock_freq; > > I feel it is okay to have this parameter which provides provision for > spec parameters to be hard-coded instead of calculating if a panel needs > and this is *only* such parameter and I already have ways to remove it > as well. IMHO it is a small price to pay to get one generic panel driver. > > That is another thing if the DSI/DPHy specs themselves evolve and we > need to modify our core, but that is unavoidable I guess. > >> >> A model similar to what I'm proposing has also been tried and tested, >> with several panels: drivers/video/omap2/. It's not DRM, and the control >> is in the panel drivers, but the separation is extremely clear (panel >> drivers are separate kernel modules). > > Understand, but again my idea is to have one single driver which can > work for all panels and hence there is no need of multiple panel > drivers. This works with SoC side of the framework. I have not yet > described how we can achieve this, but first does it has any merit to > have something like this ? My educated guess is that a single panel driver is not going to work for *all* panels, *but* it is of course benefitial to share each panel driver for as many panels as technically makes sense. So yes, I agree there's merit in having a panel driver that works on a bunch of panels based on parameters from the VBT. Apologies if it sounded like I'm against that. Based on my (limited, but not insignificant) experience with DSI panels, you can't parametrize everything, and you will need panel specific code to handle them. I presume there will also be cases where we won't have the parameters in VBT. The DSI specs do not cover everything, and some panels are outright out of spec. And then there are DSI->LVDS bridges etc. needing special attention. So I think let's keep trying to find the right abstractions to separate the DSI core and the panel drivers, make it possible to support several panels with one driver, and make it possible to have independent drivers for panels that don't fit the assumptions of the generic panel driver. Does that conflict with your goals? Are we in agreement here? >> No doubt the clear separation between the core and the panel drivers >> will be harder and more work in the short term, but it will pay off in >> the long term. And it doesn't all have to happen at once, as long as we >> work *towards* that goal, not away from it. > > I think we should take into account the amount of effort required to > develop and maintain bridge drivers for tens of MIPI panel out there Vs > having one panel driver to maintain and make fully spec compliant taking > care of open ends left by the specs in the best way we can to achieve > this generality. See above. I'm not proposing you split it all out to separate drivers. Again, I'm sorry if I haven't been clear about that. Now, the question is how do we achieve all this. First things first, let's try to get any straightforward changes merged (small patches!), so we can at least narrow the gap to upstream. And I need to have a look at your panel driver code. BR, Jani.
On 10/23/2013 07:52 PM, Jani Nikula wrote: > On Wed, 23 Oct 2013, Shobhit Kumar <shobhit.kumar@intel.com> wrote: >> Hi Jani, >> On 10/22/2013 05:23 PM, Jani Nikula wrote: >>> On Tue, 22 Oct 2013, Shobhit Kumar <shobhit.kumar@intel.com> wrote: >>>> On 10/21/2013 6:57 PM, Jani Nikula wrote: >>>>> >>>>> Hi Shobhit - >>>>> >>>>> On Mon, 21 Oct 2013, Shobhit Kumar <shobhit.kumar@intel.com> wrote: >>>>>> Also add new fields in intel_dsi to have all dphy related parameters. >>>>>> These will be useful even when we go for pure generic MIPI design >>>>> >>>>> I feel like we have a different idea of what the ideal generic design >>>>> is. For me, the goal is that we change the struct intel_dsi_device to >>>>> struct drm_bridge, and those drm_bridge drivers are all about the panel, >>>>> and have as few details about i915 or our hardware as possible. Having >>>>> the bridge drivers fill in register values to be written by the core DSI >>>>> code does not fit that. Yes, I had some of those in my bridge conversion >>>>> patches too, but I did not intend we'd keep adding more. >>>>> >>>>> I'd rather we provide generic helpers the bridge driver can call. >>>> >>>> Yeah, look like our ideas are different. In your goal with drm_bridge >>>> architecture, we will still end up having multiple bridge drivers for >>>> each different panel. But my goal is to have a single driver which can >>>> work for multiple panels. >>> >>> I'm trying to look one or two steps further, and what it will mean to >>> the driver. Here's the long term goal in upstream as I see it: There >>> will be a framework in place that allows one to write a (DSI) panel >>> driver once, using generic APIs, and use that panel driver with any SoC >>> (that implements the other side of the framework). >> >> To clarify in terms of what we have currently, in my opinion intel_dsi.c >> is one such SoC specific implementation for BYT and associated MIPI host >> controller. And sub-encoder drivers are other end and I want to unify >> sub-encoder side to have just one sub-encoder for any panel out there. >> What you are talking makes perfect sense if we are going to have each >> panel driver as a separate driver. But we can still achieve this >> separation with common panel driver as well. >> >>> >>> We are obviously far away from that goal at the moment. But IMHO we >>> should keep that in mind as a guide to what we are doing now. Moving >>> towards a model with a clearly defined API between the DSI core and the >>> panels, where the panel specific things are abstracted away from the >>> core, or towards a model where the core and panel driver depend on the >>> implementation of each other, communicating via variables. >> >> I think we are aligned on the goal and I feel there is a need for common >> DSI core to be separate >> >>> >>>> Since we already have enabled some panels with sub-encoder >>>> architecture for completeness I was planning to maintain generic >>>> driver as one sub-encoder. >>> >>> Nothing prevents you from doing that, as long as the separation between >>> the core and the panel drivers remains clear. >>> >>>> But actually we can do away with all sub-encoder and do not need even >>>> drm_bridge and all implementation will be in intel_dsi.c. Panel >>>> specific configurations or sequences will come from VBT which I have >>>> tried to convert as parameters. >>> >>> With this model it is all too easy to forget what is the panel driver >>> and what is the SoC driver. They *are* two separate things, and should >>> not be mixed. It will be all too easy to keep adding new parameters and >>> conditions in the code as new panel drivers appear to need them. It will >>> lead to code that is very difficult to understand and maintain. >> >> I think here you have misunderstood my proposal. I still treat SoC >> driver and actual Panel driver as separate. And whatever parameters I >> have tried to add are all DSI/DPHY spec related. There is not even >> single parameter which is panel specific. If you are confusing this >> because of use of - >> >> if (intel_dsi->dsi_clock_freq) >> dsi_clk = intel_dsi->dsi_clock_freq; >> >> I feel it is okay to have this parameter which provides provision for >> spec parameters to be hard-coded instead of calculating if a panel needs >> and this is *only* such parameter and I already have ways to remove it >> as well. IMHO it is a small price to pay to get one generic panel driver. >> >> That is another thing if the DSI/DPHy specs themselves evolve and we >> need to modify our core, but that is unavoidable I guess. >> >>> >>> A model similar to what I'm proposing has also been tried and tested, >>> with several panels: drivers/video/omap2/. It's not DRM, and the control >>> is in the panel drivers, but the separation is extremely clear (panel >>> drivers are separate kernel modules). >> >> Understand, but again my idea is to have one single driver which can >> work for all panels and hence there is no need of multiple panel >> drivers. This works with SoC side of the framework. I have not yet >> described how we can achieve this, but first does it has any merit to >> have something like this ? > > My educated guess is that a single panel driver is not going to work for > *all* panels, *but* it is of course benefitial to share each panel > driver for as many panels as technically makes sense. > > So yes, I agree there's merit in having a panel driver that works on a > bunch of panels based on parameters from the VBT. Apologies if it > sounded like I'm against that. > > Based on my (limited, but not insignificant) experience with DSI panels, > you can't parametrize everything, and you will need panel specific code > to handle them. I presume there will also be cases where we won't have > the parameters in VBT. The DSI specs do not cover everything, and some > panels are outright out of spec. And then there are DSI->LVDS bridges > etc. needing special attention. In my understanding there are two things - one is configuration parameters and the sencond is panel specific sequneces for say enable/disable itself. To support both of them there is a design update in VBT as such. So far as per our experience till now on whatever panels we have enabled, we could cover them with common driver + VBT. But you are right there might be some really quirky panels which might fall out of this driver and that is why I am all for having sub-encoder design as is today and bridge driver in near future and want to ensure that the generic driver is as per the sub-encoder for now and drm_bridge soon. Initially I was only aiming for native MIPI, but now I see some needs for DSI->LVDS bridge also. So this is a good point to consider as well. > > So I think let's keep trying to find the right abstractions to separate > the DSI core and the panel drivers, make it possible to support several > panels with one driver, and make it possible to have independent drivers > for panels that don't fit the assumptions of the generic panel driver. > > Does that conflict with your goals? Are we in agreement here? Definetely we are in agreement and perfectly aligns with my goals. But is it okay to work towards pushing sub-encoder based design for immidiate short term and then work to convert on drm_bridge because I can see that drm_bridge callbacks will need additions defintely and it might take some time to get that done. In the meantime can we push current driver with already suggested changes to get atleast a working base ? > >>> No doubt the clear separation between the core and the panel drivers >>> will be harder and more work in the short term, but it will pay off in >>> the long term. And it doesn't all have to happen at once, as long as we >>> work *towards* that goal, not away from it. >> >> I think we should take into account the amount of effort required to >> develop and maintain bridge drivers for tens of MIPI panel out there Vs >> having one panel driver to maintain and make fully spec compliant taking >> care of open ends left by the specs in the best way we can to achieve >> this generality. > > See above. I'm not proposing you split it all out to separate > drivers. Again, I'm sorry if I haven't been clear about that. > > > Now, the question is how do we achieve all this. First things first, > let's try to get any straightforward changes merged (small patches!), so > we can at least narrow the gap to upstream. And I need to have a look at > your panel driver code. Sure. And as I propsed above we can push smaller cleaner patches for current design itself to get working base. Generic panel driver comes after that. Regards Shobhit
On Thu, 24 Oct 2013, Shobhit Kumar <shobhit.kumar@intel.com> wrote: > On 10/23/2013 07:52 PM, Jani Nikula wrote: >> So I think let's keep trying to find the right abstractions to separate >> the DSI core and the panel drivers, make it possible to support several >> panels with one driver, and make it possible to have independent drivers >> for panels that don't fit the assumptions of the generic panel driver. >> >> Does that conflict with your goals? Are we in agreement here? > > Definetely we are in agreement and perfectly aligns with my goals. That's relieving, I'm happy we're on the same page now. :) > But is it okay to work towards pushing sub-encoder based design for > immidiate short term and then work to convert on drm_bridge because I > can see that drm_bridge callbacks will need additions defintely and it > might take some time to get that done. In the meantime can we push > current driver with already suggested changes to get atleast a working > base ? I'm okay with this. Daniel is pushing for drm_bridge, and I'm also optimistic about that, but perhaps we have to see what we really need first. The current sub-encoder model is more flexible for that in the short term. However I, and others, will need to know where we are heading, so please do pay attention to splitting up the patches and explaining why they are needed. Sometimes it's helpful to provide draft/RFC patches on top just for that. Finally, I am glad you're contributing directly to upstream now. It makes a huge difference in the long run. BR, Jani.
On 10/24/2013 01:54 PM, Jani Nikula wrote: > On Thu, 24 Oct 2013, Shobhit Kumar <shobhit.kumar@intel.com> wrote: >> On 10/23/2013 07:52 PM, Jani Nikula wrote: >>> So I think let's keep trying to find the right abstractions to separate >>> the DSI core and the panel drivers, make it possible to support several >>> panels with one driver, and make it possible to have independent drivers >>> for panels that don't fit the assumptions of the generic panel driver. >>> >>> Does that conflict with your goals? Are we in agreement here? >> >> Definetely we are in agreement and perfectly aligns with my goals. > > That's relieving, I'm happy we're on the same page now. :) Me too :) > >> But is it okay to work towards pushing sub-encoder based design for >> immidiate short term and then work to convert on drm_bridge because I >> can see that drm_bridge callbacks will need additions defintely and it >> might take some time to get that done. In the meantime can we push >> current driver with already suggested changes to get atleast a working >> base ? > > I'm okay with this. Daniel is pushing for drm_bridge, and I'm also > optimistic about that, but perhaps we have to see what we really need > first. The current sub-encoder model is more flexible for that in the > short term. > > However I, and others, will need to know where we are heading, so please > do pay attention to splitting up the patches and explaining why they are > needed. Sometimes it's helpful to provide draft/RFC patches on top just > for that. I will push updated smaller patches with clear reasoning for them. > > Finally, I am glad you're contributing directly to upstream now. It > makes a huge difference in the long run. > Yeah, I wanted to do earlier but sometimes its all matter of bandwidth and priorities. Hopefully I will be able to continue to do so, now that I have started. Thanks for all your inputs. Regards Shobhit
diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c index 9a2fdd2..34e19b7 100644 --- a/drivers/gpu/drm/i915/intel_dsi.c +++ b/drivers/gpu/drm/i915/intel_dsi.c @@ -147,6 +147,9 @@ static void intel_dsi_enable(struct intel_encoder *encoder) DRM_DEBUG_KMS("\n"); + if (intel_dsi->dev.dev_ops->panel_reset) + intel_dsi->dev.dev_ops->panel_reset(&intel_dsi->dev); + temp = I915_READ(MIPI_DEVICE_READY(pipe)); if ((temp & DEVICE_READY) == 0) { temp &= ~ULPS_STATE_MASK; @@ -162,6 +165,9 @@ static void intel_dsi_enable(struct intel_encoder *encoder) I915_WRITE(MIPI_DEVICE_READY(pipe), temp); } + if (intel_dsi->dev.dev_ops->send_otp_cmds) + intel_dsi->dev.dev_ops->send_otp_cmds(&intel_dsi->dev); + if (is_cmd_mode(intel_dsi)) I915_WRITE(MIPI_MAX_RETURN_PKT_SIZE(pipe), 8 * 4); @@ -176,7 +182,8 @@ static void intel_dsi_enable(struct intel_encoder *encoder) POSTING_READ(MIPI_PORT_CTRL(pipe)); } - intel_dsi->dev.dev_ops->enable(&intel_dsi->dev); + if (intel_dsi->dev.dev_ops->enable) + intel_dsi->dev.dev_ops->enable(&intel_dsi->dev); } static void intel_dsi_disable(struct intel_encoder *encoder) diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h index c7765f3..b71c9b3 100644 --- a/drivers/gpu/drm/i915/intel_dsi.h +++ b/drivers/gpu/drm/i915/intel_dsi.h @@ -39,6 +39,13 @@ struct intel_dsi_device { struct intel_dsi_dev_ops { bool (*init)(struct intel_dsi_device *dsi); + void (*panel_reset)(struct intel_dsi_device *dsi); + + void (*disable_panel_power)(struct intel_dsi_device *dsi); + + /* send one time programmable commands */ + void (*send_otp_cmds)(struct intel_dsi_device *dsi); + /* This callback must be able to assume DSI commands can be sent */ void (*enable)(struct intel_dsi_device *dsi); @@ -89,6 +96,28 @@ struct intel_dsi { /* eot for MIPI_EOT_DISABLE register */ u32 eot_disable; + + u16 dsi_clock_freq; + u8 video_mode_type; + u32 data_width; + u8 dither; + u32 port_bits; + u8 escape_clk_div; + u32 lp_rx_timeout; + u8 turn_arnd_val; + u16 init_count; + u16 rst_timer_val; + u16 hs_to_lp_count; + u16 lp_byte_clk; + u32 bw_timer; + u16 clk_lp_to_hs_count; + u16 clk_hs_to_lp_count; + u32 video_frmt_cfg_bits; + u32 dphy_reg; + + u8 backlight_off_delay; /*in ms*/ + bool send_shutdown; + u8 shutdown_pkt_delay; /*in ms*/ }; static inline struct intel_dsi *enc_to_intel_dsi(struct drm_encoder *encoder)
Also add new fields in intel_dsi to have all dphy related parameters. These will be useful even when we go for pure generic MIPI design Yogesh Mohan Marimuthu <yogesh.mohan.marimuthu@intel.com> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com> --- drivers/gpu/drm/i915/intel_dsi.c | 9 ++++++++- drivers/gpu/drm/i915/intel_dsi.h | 29 +++++++++++++++++++++++++++++ 2 files changed, 37 insertions(+), 1 deletion(-)