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.
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
