Re: [PATCH v3 1/7] drm: Add DSI bus infrastructure

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2643 bytes --]

On Thu, Nov 14, 2013 at 03:04:19PM +0000, Bert Kenward wrote:
> On 11/14/2013 14:16, Andrzej Hajda wrote:
> > On 11/13/2013 10:38 PM, Thierry Reding wrote:
> > >  Furthermore I think if we kept the transfer function proposed
> > > in my patch should make it easier to address Bert's comments from your
> > > posting.
> > I am not sure which part of Barts comment you are addressing.
> > Anyway I also prefer passing struct and returning ssize_t.
> > I am not sure about splitting type and channel but this seems to be a
> > minor detail.
> 
> Most of the comments I made about Andrzej's patch from last month
> apply here, and would result in extensions to struct dsi_msg. Some
> means of choosing whether the transfer should be in HS or LP mode is
> necessary. For video mode panels some means of specifying a period
> (VFP, VBP, etc) for sending the transfer is needed. Perhaps struct
> dsi_msg should include:
> 
> #define DSI_WINDOW_VFP  (1 << 0)
> #define DSI_WINDOW_ACT  (1 << 1)
> #define DSI_WINDOW_VBP  (1 << 2)
> #define DSI_WINDOW_VSY  (1 << 3)
> 
> /**
>  * struct dsi_msg - DSI command message
>  * @channel: virtual channel to send the message to
>  * @type: data ID of the message
>  * @lp_mode: send in LP mode if non-zero

Perhaps a flags field would be more flexible here. I can easily imagine
other I can imagine that other flags may be needed eventually.

>  * @window: video period when transfer is allowed - bitmask of DSI_WINDOW_*

I'm not sure if this is the right interface. What will happen for
instance if the hardware doesn't support any of the bits in that mask?
Perhaps a slightly better approach might be to expose the capabilities
of the DSI host, so that the DSI core knows up front which windows can
be used.

>  * @tx_len: length of transmission buffer
>  * @tx: transmission buffer
>  * @rx_len: length of reception buffer
>  * @rx: reception buffer
>  */
> struct dsi_msg {
> 	u8 channel;
> 	u8 type;
> 	u8 lp_mode;
> 	u8 window;
> 
> 	size_t tx_len;
> 	void *tx;
> 
> 	size_t rx_len;
> 	void *rx;
> };
> 
> The ability to specify synchronisation with a tearing effect control
> signal for a command mode panel is obviously important. It's somewhat
> analogous to the "window" option for video mode.
> 
> They're little used, but a mechanism for sending triggers should be
> included. They're probably sufficiently different that it should be a
> different op.

I agree. While I currently have no use-case where this is required, I
think having a struct dsi_msg makes it easier to extend the featureset
on an as-needed basis.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel