Re: [PATCH 3/5] drm/i2c: adv7511: Refactor encoder slave functions

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Boris, Laurent,

On 07/28/2015 08:08 PM, Boris Brezillon wrote:
Archit, Laurent,

On Tue, 28 Jul 2015 13:47:37 +0530
Archit Taneja <architt@xxxxxxxxxxxxxx> wrote:

Hi,

On 07/27/2015 02:29 PM, Laurent Pinchart wrote:
Hi Archit,

(CC'ing Boris Brezillon)

Thank you for the patch.

On Monday 27 July 2015 11:46:57 Archit Taneja wrote:
ADV7511 is represented as an i2c drm slave encoder device. ADV7533, on
the other hand, is going be a normal i2c client device creating bridge
and connector entities.

Please, no. It's really time to stop piling hacks and fix the problem
properly. There's no reason to have separate APIs for I2C slave encoders and
DRM bridges. Those two APIs need to be merged, and then you'll find it much
easier to implement ADV7533 support.

i2c slave encoders and bridges aren't exactly the same. slave encoders
are used when the there is no real 'encoder' in the display chain.
bridges are used when there is already an encoder available, and the
bridge entity represents another encoder in the chain.

ADV7511 takes in RGB/MIPI DPI data, which is generally the output of a
crtc for drm drivers.

ADV7533 takes in MIPI DSI data, which is generally the output of an
encoder for drm drivers.

Therefore, having i2c slave encoder for the former and bridge for the
latter made sense to me.

I do agree that it would be better if they were somehow merged. It
would prevent the fragmentation we currently have among encoder
chips.

One possible way would be to convert slave encoder to bridge. With
this, an i2c slave encoder would be a 'dummy encoder' and a bridge.
i2c slave encoders even now just tie the slave encoder helper funcs
to encoder helper funcs. So it's not really any different.

Merging these 2 entities would be nice, but we're still shying away
from the larger problem of creating generic encoder chains. The
ideal solution would be for bridges and slave encoders to just be
'encoders', and the facility to connect on encoder output to the
input of another. I don't know how easy it is to do this, and
whether it'll break userspace.

Yes, that's pretty much what I was trying to do.
I'd also like to ease display pipelines creation by providing helper
functions, so that display controller don't have to worry about encoders
and connectors creation if these ones are attached to external encoders.


Archit


Boris, I know you were experimenting with that, do you have anything to report
?

Nope, I didn't work on it since last time we talked about it. I pushed
my work here if you want to have a look [1].

I went through the branch you shared. From what I understood, the encoder chain comprises of one 'real' encoder object (drm_encoder) in the 'drm_encoder_chain' struct. This drm_encoder encapsulates all the 'encoder elements' forming the chain.

I'm guessing the various dridge/slave encoder drivers would have to be changed to now create a drm_encoder_element object, replacing drm_bridge/drm_i2c_slave_encoder objects.

One problem I see with this approach is that we can't use this when
the display controller already exposes a drm_encoder. I.e, it already
creates a drm_encoder object. If we want the encoder chain to be
connected to the output of this encoder, we'll need to link the 2
drm_encoders together, which isn't possible at the moment.

I guess we have two ways to go about this:

1) Have an approach like this where all the entities in the encoder
chain connect to just one encoder. We define the sequence in which
they are connected. The drm kms framework acts as if this chain
doesn't exist, and assumes that this is what the encoder is
outputting.

2) Make each element in the chain be a 'drm_encoder' object in itself. This would be a more intrusive change, since drm_encoders are expected to receive an input from crtc, and output to a connector. Furthermore, it may confuse userspace what encoder to chose.

For 1), we could either work more on your approach, or use drm_bridges. drm_bridges can already be chained to each other, and bridge ops of each bridge in the chain are called successively. It still relies
on the device drivers to form the chain, which is something your
approach takes care of by itself. But that's something that can be
extended for bridges too.

Laurent,

Merging i2c slave encoders and bridges is possible, but there is no
guarantee that the new solution would be future proof and work well
with encoder chains. We needed more consensus from folks on
dri-devel.

For ADV7533, could you please look at the other parts? Especially the
one that creates a dummy mipi DSI device, and connecting to a dsi
host. I'd previously posted a RFC to enable that:

http://lkml.iu.edu/hypermail/linux/kernel/1506.3/03249.html

Archit


Best Regards,

Boris

[1]https://github.com/bbrezillon/linux-at91/tree/drm-encoder-chain-WIP




--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux