Re: [RESEND PATCH v5 3/3] drm/atmel-hlcdc: support bus-width (12/16/18/24) in endpoint nodes

From: jacopo mondi
Date: Fri Aug 03 2018 - 04:18:06 EST

Hi Peter,

On Fri, Aug 03, 2018 at 09:23:08AM +0200, Peter Rosin wrote:
> This beats the heuristic that the connector is involved in what format
> should be output for cases where this fails.
>
> E.g. if there is a bridge that changes format between the encoder and the
> connector, or if some of the RGB pins between the lcd controller and the
> encoder are not routed on the PCB.
>
> This is critical for the devices that have the "conflicting output
> formats" issue (SAM9N12, SAM9X5, SAMA5D3), since the most significant
> RGB bits move around depending on the selected output mode. For
> devices that do not have the "conflicting output formats" issue
> (SAMA5D2, SAMA5D4), this is completely irrelevant.
>
> Acked-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxx>
> Signed-off-by: Peter Rosin <peda@xxxxxxxxxx>
> ---
> drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 70 +++++++++++++++++-------
> drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h | 1 +
> drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c | 67 ++++++++++++++++++++---
> 3 files changed, 111 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> index d73281095fac..c38a479ada98 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> @@ -226,6 +226,55 @@ static void atmel_hlcdc_crtc_atomic_enable(struct drm_crtc *c,
> #define ATMEL_HLCDC_RGB888_OUTPUT BIT(3)
> #define ATMEL_HLCDC_OUTPUT_MODE_MASK GENMASK(3, 0)
>
> +static int atmel_hlcdc_connector_output_mode(struct drm_connector_state *state)
> +{
> + struct drm_connector *connector = state->connector;
> + struct drm_display_info *info = &connector->display_info;
> + struct drm_encoder *encoder;
> + unsigned int supported_fmts = 0;
> + int j;
> +
> + encoder = state->best_encoder;
> + if (!encoder)
> + encoder = connector->encoder;
> +
> + switch (atmel_hlcdc_encoder_get_bus_fmt(encoder)) {
> + case 0:
> + break;
> + case MEDIA_BUS_FMT_RGB444_1X12:
> + return ATMEL_HLCDC_RGB444_OUTPUT;
> + case MEDIA_BUS_FMT_RGB565_1X16:
> + return ATMEL_HLCDC_RGB565_OUTPUT;
> + case MEDIA_BUS_FMT_RGB666_1X18:
> + return ATMEL_HLCDC_RGB666_OUTPUT;
> + case MEDIA_BUS_FMT_RGB888_1X24:
> + return ATMEL_HLCDC_RGB888_OUTPUT;
> + default:
> + return -EINVAL;
> + }
> +
> + for (j = 0; j < info->num_bus_formats; j++) {
> + switch (info->bus_formats[j]) {
> + case MEDIA_BUS_FMT_RGB444_1X12:
> + supported_fmts |= ATMEL_HLCDC_RGB444_OUTPUT;
> + break;
> + case MEDIA_BUS_FMT_RGB565_1X16:
> + supported_fmts |= ATMEL_HLCDC_RGB565_OUTPUT;
> + break;
> + case MEDIA_BUS_FMT_RGB666_1X18:
> + supported_fmts |= ATMEL_HLCDC_RGB666_OUTPUT;
> + break;
> + case MEDIA_BUS_FMT_RGB888_1X24:
> + supported_fmts |= ATMEL_HLCDC_RGB888_OUTPUT;
> + break;
> + default:
> + break;
> + }
> + }
> +
> + return supported_fmts;
> +}
> +
> static int atmel_hlcdc_crtc_select_output_mode(struct drm_crtc_state *state)
> {
> unsigned int output_fmts = ATMEL_HLCDC_OUTPUT_MODE_MASK;
> @@ -238,31 +287,12 @@ static int atmel_hlcdc_crtc_select_output_mode(struct drm_crtc_state *state)
> crtc = drm_crtc_to_atmel_hlcdc_crtc(state->crtc);
>
> for_each_new_connector_in_state(state->state, connector, cstate, i) {
> - struct drm_display_info *info = &connector->display_info;
> unsigned int supported_fmts = 0;
> - int j;
>
> if (!cstate->crtc)
> continue;
>
> - for (j = 0; j < info->num_bus_formats; j++) {
> - switch (info->bus_formats[j]) {
> - case MEDIA_BUS_FMT_RGB444_1X12:
> - supported_fmts |= ATMEL_HLCDC_RGB444_OUTPUT;
> - break;
> - case MEDIA_BUS_FMT_RGB565_1X16:
> - supported_fmts |= ATMEL_HLCDC_RGB565_OUTPUT;
> - break;
> - case MEDIA_BUS_FMT_RGB666_1X18:
> - supported_fmts |= ATMEL_HLCDC_RGB666_OUTPUT;
> - break;
> - case MEDIA_BUS_FMT_RGB888_1X24:
> - supported_fmts |= ATMEL_HLCDC_RGB888_OUTPUT;
> - break;
> - default:
> - break;
> - }
> - }
> + supported_fmts = atmel_hlcdc_connector_output_mode(cstate);
>
> if (crtc->dc->desc->conflicting_output_formats)
> output_fmts &= supported_fmts;
> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h
> index 60c937f42114..4cc1e03f0aee 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h
> @@ -441,5 +441,6 @@ void atmel_hlcdc_crtc_irq(struct drm_crtc *c);
> int atmel_hlcdc_crtc_create(struct drm_device *dev);
>
> int atmel_hlcdc_create_outputs(struct drm_device *dev);
> +int atmel_hlcdc_encoder_get_bus_fmt(struct drm_encoder *encoder);
>
> #endif /* DRM_ATMEL_HLCDC_H */
> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
> index 8db51fb131db..db4d6aaaef93 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
> @@ -27,33 +27,86 @@
>
> #include "atmel_hlcdc_dc.h"
>
> +struct atmel_hlcdc_rgb_output {
> + struct drm_encoder encoder;
> + int bus_fmt;
> +};
> +
> static const struct drm_encoder_funcs atmel_hlcdc_panel_encoder_funcs = {
> .destroy = drm_encoder_cleanup,
> };
>
> +static struct atmel_hlcdc_rgb_output *
> +atmel_hlcdc_encoder_to_rgb_output(struct drm_encoder *encoder)
> +{
> + return container_of(encoder, struct atmel_hlcdc_rgb_output, encoder);
> +}
> +
> +int atmel_hlcdc_encoder_get_bus_fmt(struct drm_encoder *encoder)

'static' missing?

> +{
> + struct atmel_hlcdc_rgb_output *output;
> +
> + output = atmel_hlcdc_encoder_to_rgb_output(encoder);
> +
> + return output->bus_fmt;
> +}
> +
> +static int atmel_hlcdc_of_bus_fmt(struct device_node *ep)
> +{
> + u32 bus_width = 0;
> +
> + of_property_read_u32(ep, "bus-width", &bus_width);
> +
> + switch (bus_width) {
> + case 0:
> + return 0;
> + case 12:
> + return MEDIA_BUS_FMT_RGB444_1X12;
> + case 16:
> + return MEDIA_BUS_FMT_RGB565_1X16;
> + case 18:
> + return MEDIA_BUS_FMT_RGB666_1X18;
> + case 24:
> + return MEDIA_BUS_FMT_RGB888_1X24;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> static int atmel_hlcdc_attach_endpoint(struct drm_device *dev, int endpoint)
> {
> - struct drm_encoder *encoder;
> + struct atmel_hlcdc_rgb_output *output;
> + struct device_node *ep;
> struct drm_panel *panel;
> struct drm_bridge *bridge;
> int ret;
>
> + ep = of_graph_get_endpoint_by_regs(dev->dev->of_node, 0, endpoint);
> + if (!ep)
> + return -ENODEV;
> +
> ret = drm_of_find_panel_or_bridge(dev->dev->of_node, 0, endpoint,
> &panel, &bridge);
> if (ret)
> return ret;
>
> - encoder = devm_kzalloc(dev->dev, sizeof(*encoder), GFP_KERNEL);
> - if (!encoder)
> + output = devm_kzalloc(dev->dev, sizeof(*output), GFP_KERNEL);
> + if (!output)
> return -EINVAL;
>
> - ret = drm_encoder_init(dev, encoder,
> + output->bus_fmt = atmel_hlcdc_of_bus_fmt(ep);
> + if (output->bus_fmt < 0) {
> + dev_err(dev->dev, "invalid bus width\n");

Nit: Bindings do not specify 0 as an accepted value.

Feel free to add my
Reviewed-by: Jacopo Mondi <jacopo+renesas@xxxxxxxxxx>
with those two minors addressed.

Thanks
j
> + return -EINVAL;
> + }
> +
> + ret = drm_encoder_init(dev, &output->encoder,
> &atmel_hlcdc_panel_encoder_funcs,
> DRM_MODE_ENCODER_NONE, NULL);
> if (ret)
> return ret;
>
> - encoder->possible_crtcs = 0x1;
> + output->encoder.possible_crtcs = 0x1;
>
> if (panel) {
> bridge = drm_panel_bridge_add(panel, DRM_MODE_CONNECTOR_Unknown);
> @@ -62,7 +115,7 @@ static int atmel_hlcdc_attach_endpoint(struct drm_device *dev, int endpoint)
> }
>
> if (bridge) {
> - ret = drm_bridge_attach(encoder, bridge, NULL);
> + ret = drm_bridge_attach(&output->encoder, bridge, NULL);
> if (!ret)
> return 0;
>
> @@ -70,7 +123,7 @@ static int atmel_hlcdc_attach_endpoint(struct drm_device *dev, int endpoint)
> drm_panel_bridge_remove(bridge);
> }
>
> - drm_encoder_cleanup(encoder);
> + drm_encoder_cleanup(&output->encoder);
>
> return ret;
> }
> --
> 2.11.0
>

Attachment: signature.asc
Description: PGP signature