[Intel-gfx] [PATCH 1/4] drm/i915: Add more dev ops for MIPI sub encoder

[Shobhit Kumar]

Hi Jani,
On 10/22/2013 05:23 PM, Jani Nikula wrote:
> On Tue, 22 Oct 2013, Shobhit Kumar <shobhit.kumar at intel.com> wrote:
>> On 10/21/2013 6:57 PM, Jani Nikula wrote:
>>>
>>> Hi Shobhit -
>>>
>>> On Mon, 21 Oct 2013, Shobhit Kumar <shobhit.kumar at 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