[PATCH] drm/imx: parallel-display: Adjust bus_flags and bus_format handling

[Marek Vasut]

On 3/9/20 9:03 PM, Marek Vasut wrote:
> On 3/9/20 11:50 AM, Philipp Zabel wrote:
>> Hi Marek,
> 
> Hi,
> 
>> On Thu, 2019-11-14 at 14:17 +0100, Marek Vasut wrote:
>>> The bus_flags and bus_format handling logic does not seem to cover
>>> all potential usecases. Specifically, this seems to fail with an
>>> "edt,etm0700g0edh6" display attached to an 24bit display interface,
>>> with interface-pix-fmt = "rgb24" set in DT.
>>
>> interface-pix-fmt is a legacy property that was never intended to be
>> used as an override for the panel bus format. The bus flags were
>> supposed to be set from the display-timings node, back when there was no
>> of-graph connected panel at all.
>>
>> That being said, there isn't really a proper alternative that allows to
>> override the bus format requested by the panel driver in the device tree
>> to account for weird wiring. We could reuse the bus-width endpoint
>> property documented in [1], but that wouldn't completely specify how the
>> RGB components are to be mapped onto the parallel bus.
>>
>> [1] Documentation/devicetree/bindings/media/video-interfaces.txt
>>
>> I do wonder whether for your case it would be better to implement a
>> MEDIA_BUS_FMT_RGB666_1X24_CPADLO though, to leave the LSBs untouched
>> instead of risking to dump them into accidental PCB antennae.
>>
>>> In this specific setup, the panel-simple.c driver entry for the display
>>> sets .bus_flags to non-zero value. However, as imxpd->bus_format is set
>>> from the DT property "interface-pix-fmt", imx_pd_encoder_atomic_check()
>>> will set imx_crtc_state->bus_flags = imxpd->bus_flags even though the
>>> imxpd->bus_flags is zero, while the di->bus_flags is correctly set by
>>> the panel-simple.c and non-zero.
>>>
>>> The result is incorrect flags being
>>> used for the display configuration and thus an image corruption.
>>> (Specifically, DRM_BUS_FLAG_PIXDATA_POSEDGE is not propagated and thus
>>> the ipuv3 clocks pixels on the wrong edge).
>>>
>>> This patch fixes the problem by overriding the imx_crtc_state->bus_format
>>> from the imxpd->bus_format only if the DT property "interface-pix-fmt" is
>>> present or if the DI provides no formats. Similarly for bus_flags, which
>>> are set from imxpd->bus_flags only if the DI provides no formats.
>>>
>>> Signed-off-by: Marek Vasut <marex at denx.de>
>>> Cc: Daniel Vetter <daniel at ffwll.ch>
>>> Cc: David Airlie <airlied at linux.ie>
>>> Cc: Fabio Estevam <festevam at gmail.com>
>>> Cc: NXP Linux Team <linux-imx at nxp.com>
>>> Cc: Philipp Zabel <p.zabel at pengutronix.de>
>>> Cc: Sascha Hauer <s.hauer at pengutronix.de>
>>> Cc: Shawn Guo <shawnguo at kernel.org>
>>> Cc: linux-arm-kernel at lists.infradead.org
>>> To: dri-devel at lists.freedesktop.org
>>> ---
>>>  drivers/gpu/drm/imx/parallel-display.c | 13 ++++++++-----
>>>  1 file changed, 8 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c
>>> index 35518e5de356..92f00b12c068 100644
>>> --- a/drivers/gpu/drm/imx/parallel-display.c
>>> +++ b/drivers/gpu/drm/imx/parallel-display.c
>>> @@ -113,13 +113,16 @@ static int imx_pd_encoder_atomic_check(struct drm_encoder *encoder,
>>>  	struct drm_display_info *di = &conn_state->connector->display_info;
>>>  	struct imx_parallel_display *imxpd = enc_to_imxpd(encoder);
>>>  
>>> -	if (!imxpd->bus_format && di->num_bus_formats) {
>>> -		imx_crtc_state->bus_flags = di->bus_flags;
>>> +	if (imxpd->bus_format || !di->num_bus_formats)
>>
>> I see no reason to invert the logic here. Let's keep the common case
>> first.
>>
>>> +		imx_crtc_state->bus_format = imxpd->bus_format;
>>> +	else
>>>  		imx_crtc_state->bus_format = di->bus_formats[0];
>>> -	} else {
>>> +
>>> +	if (di->num_bus_formats)
>>> +		imx_crtc_state->bus_flags = di->bus_flags;
>>> +	else
>>>  		imx_crtc_state->bus_flags = imxpd->bus_flags;
>>> -		imx_crtc_state->bus_format = imxpd->bus_format;
>>> -	}
>>> +
>>>  	imx_crtc_state->di_hsync_pin = 2;
>>>  	imx_crtc_state->di_vsync_pin = 3;
>>
>> So while I don't like this being used at all, I think the patch improves
>> consistency, as imx_pd_connector_get_modes doesn't allow to override the
>> panel's modes with DT display-timings either.
> 
> So when this patch was posted half a year ago, it was needed. I rebased
> on current next and this patch is no longer needed as some form of it
> got in as part of other patches. This functionality is still broken in
> e.g. 5.4 though.

Correction, a small subset of this patch is still needed, I'll send a V2.