Re: [PATCH 0/3] imx drm atomic mode setting fixups

From: Philipp Zabel <p.zabel@pengutronix.de>
To: Ying Liu <gnuiyl@gmail.com>
Cc: Russell King <rmk+kernel@arm.linux.org.uk>,
	kernel@pengutronix.de,
	DRI Development <dri-devel@lists.freedesktop.org>,
	Daniel Vetter <daniel.vetter@ffwll.ch>
Subject: Re: [PATCH 0/3] imx drm atomic mode setting fixups
Date: Thu, 07 Jul 2016 10:29:49 +0200	[thread overview]
Message-ID: <1467880189.4236.13.camel@pengutronix.de> (raw)
In-Reply-To: <CAOcKUNUhL7U=sX_RZTd-5yqpxLHPVoLwzyZymeJ9kty_+yavtQ@mail.gmail.com>

Hi Liu,

Am Donnerstag, den 07.07.2016, 15:44 +0800 schrieb Ying Liu:
> Hi Philipp,
> 
> On Wed, Jul 6, 2016 at 11:52 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > Hi,
> >
> > I have tested Liu Ying's imx drm atomic mode setting conversion series [1]
> 
> Thanks for the testing!
[...]
> I didn't program imx_ldb_ch->imx_encoder.bus_format correctly
> in imx_ldb_connector_get_modes() - I forgot to translate the bus format
> from LVDS bus format to internal bus format.  Also, data width and
> LVDS data mapping(SPWG/JEIDA) can be set to ldb->ldb_ctrl in
> the function. So, one option to fix the issue you found is to simply
> modify the function and respin the particular patch in my patch set.
> Actually, I have got a fix tested with the format determined by the panel.
> 
> I don't have strong opinion on storing bus_format, bus_flags and
> di_*sync_pin in imx_crtc_state.  But, very likely, they will not be
> dynamically changed.  Setting them again and again at atomic_check
> is somewhat wasting CPU cycles...

That really only should be a concern if you can measure the difference.
Further, with upcoming bridge support in the parallel-display and
imx-ldb drivers the encoder doesn't have direct access to the connector
anymore (get_modes is handled by the bridges' connector). I could also
think of bridges where the optimal input bus_format depends on the mode.
So I'd prefer the internal bus configuration to reside in imx_crtc_state
instead of imx_encoder.

> BTW, the imx-drm atomic conversion patch set has reached version 3
> which uses the new non-blocking atomic commit helpers.

Yes, thank you for the update. I chose the wrong link, I have tested
version 3. If you are willing to respin your series once more, feel free
to integrate all or part of my changes in the appropriate places (the
mode_set removal could be squashed into the "Legacy callback fixups"
patch, for example). I could then retest and potentially rebase the
remaining changes on your next version.

regards
Philipp

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