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@xxxxxxx>
>>> Cc: Daniel Vetter <daniel@xxxxxxxx>
>>> Cc: David Airlie <airlied@xxxxxxxx>
>>> Cc: Fabio Estevam <festevam@xxxxxxxxx>
>>> Cc: NXP Linux Team <linux-imx@xxxxxxx>
>>> Cc: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>
>>> Cc: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
>>> Cc: Shawn Guo <shawnguo@xxxxxxxxxx>
>>> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
>>> To: dri-devel@xxxxxxxxxxxxxxxxxxxxx
>>> ---
>>> 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.
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
