On Thu, May 11, 2017 at 09:15:23PM +0200, Boris Brezillon wrote:
Hi Eric,
On Thu, 11 May 2017 11:31:28 -0700 Eric Anholt eric@anholt.net wrote:
This cuts 135 lines of boilerplate, at the cost of losing the filtering of get_modes() using atmel_hlcdc_dc_mode_valid(). The atomic check will still check that we don't set an invalid mode, though.
Nice.
Signed-off-by: Eric Anholt eric@anholt.net
Acked-by: Boris Brezillon boris.brezillon@free-electrons.com
A few comments below (no need to address them, those are just minor things that can be fixed later on, or things I'm not comfortable with but cannot be addressed easily).
This patch is just a proposal for atmel-hlcdc if you like it -- maybe you still want the filtering of get_modes(). I'm not sure from a userspace API perspective if we should really have the encoder (your CRTC's limits) filtering modes from a connector, but I'm also pretty sure it doesn't actually matter.
Actually I added this check because someone suggested it :-). I don't think it's a real problem, and that should definitely be checked at the CRTC level, not here.
Also note that we're getting ready to merge an entire pile of ->mode_valid callbacks (from Jose Abreu), that should simplify this all a lot. With that we could stuff the mode_valid code either into the encoder or the crtc. -Daniel
drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c | 163 ++--------------------- 1 file changed, 14 insertions(+), 149 deletions(-)
diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c index 4b2cfbd0d43f..340ef962aa81 100644 --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c @@ -23,191 +23,56 @@
#include <drm/drmP.h> #include <drm/drm_of.h> +#include <drm/drm_bridge.h>
#include "atmel_hlcdc_dc.h"
-/**
- Atmel HLCDC RGB connector structure
- This structure stores RGB slave device information.
- @connector: DRM connector
- @encoder: DRM encoder
- @dc: pointer to the atmel_hlcdc_dc structure
- @panel: panel connected on the RGB output
- */
-struct atmel_hlcdc_rgb_output {
- struct drm_connector connector;
- struct drm_encoder encoder;
- struct atmel_hlcdc_dc *dc;
- struct drm_panel *panel;
-};
-static inline struct atmel_hlcdc_rgb_output * -drm_connector_to_atmel_hlcdc_rgb_output(struct drm_connector *connector) -{
- return container_of(connector, struct atmel_hlcdc_rgb_output,
connector);-}
-static inline struct atmel_hlcdc_rgb_output * -drm_encoder_to_atmel_hlcdc_rgb_output(struct drm_encoder *encoder) -{
- return container_of(encoder, struct atmel_hlcdc_rgb_output, encoder);
-}
-static void atmel_hlcdc_rgb_encoder_enable(struct drm_encoder *encoder) -{
- struct atmel_hlcdc_rgb_output *rgb =
drm_encoder_to_atmel_hlcdc_rgb_output(encoder);- if (rgb->panel) {
drm_panel_prepare(rgb->panel);drm_panel_enable(rgb->panel);- }
-}
-static void atmel_hlcdc_rgb_encoder_disable(struct drm_encoder *encoder) -{
- struct atmel_hlcdc_rgb_output *rgb =
drm_encoder_to_atmel_hlcdc_rgb_output(encoder);- if (rgb->panel) {
drm_panel_disable(rgb->panel);drm_panel_unprepare(rgb->panel);- }
-}
-static const struct drm_encoder_helper_funcs atmel_hlcdc_panel_encoder_helper_funcs = {
- .disable = atmel_hlcdc_rgb_encoder_disable,
- .enable = atmel_hlcdc_rgb_encoder_enable,
-};
static const struct drm_encoder_funcs atmel_hlcdc_panel_encoder_funcs = { .destroy = drm_encoder_cleanup, };
-static int atmel_hlcdc_panel_get_modes(struct drm_connector *connector) -{
- struct atmel_hlcdc_rgb_output *rgb =
drm_connector_to_atmel_hlcdc_rgb_output(connector);- if (rgb->panel)
return rgb->panel->funcs->get_modes(rgb->panel);- return 0;
-}
-static int atmel_hlcdc_rgb_mode_valid(struct drm_connector *connector,
struct drm_display_mode *mode)-{
- struct atmel_hlcdc_rgb_output *rgb =
drm_connector_to_atmel_hlcdc_rgb_output(connector);- return atmel_hlcdc_dc_mode_valid(rgb->dc, mode);
-}
-static const struct drm_connector_helper_funcs atmel_hlcdc_panel_connector_helper_funcs = {
- .get_modes = atmel_hlcdc_panel_get_modes,
- .mode_valid = atmel_hlcdc_rgb_mode_valid,
-};
-static enum drm_connector_status -atmel_hlcdc_panel_connector_detect(struct drm_connector *connector, bool force) -{
- struct atmel_hlcdc_rgb_output *rgb =
drm_connector_to_atmel_hlcdc_rgb_output(connector);- if (rgb->panel)
return connector_status_connected;- return connector_status_disconnected;
-}
-static void -atmel_hlcdc_panel_connector_destroy(struct drm_connector *connector) -{
- struct atmel_hlcdc_rgb_output *rgb =
drm_connector_to_atmel_hlcdc_rgb_output(connector);- if (rgb->panel)
drm_panel_detach(rgb->panel);- drm_connector_cleanup(connector);
-}
-static const struct drm_connector_funcs atmel_hlcdc_panel_connector_funcs = {
- .dpms = drm_atomic_helper_connector_dpms,
- .detect = atmel_hlcdc_panel_connector_detect,
- .fill_modes = drm_helper_probe_single_connector_modes,
- .destroy = atmel_hlcdc_panel_connector_destroy,
- .reset = drm_atomic_helper_connector_reset,
- .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
- .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
-};
static int atmel_hlcdc_attach_endpoint(struct drm_device *dev, const struct device_node *np) {
- struct atmel_hlcdc_dc *dc = dev->dev_private;
- struct atmel_hlcdc_rgb_output *output;
- struct drm_encoder *encoder; struct drm_panel *panel; struct drm_bridge *bridge; int ret;
- output = devm_kzalloc(dev->dev, sizeof(*output), GFP_KERNEL);
- if (!output)
- encoder = devm_kzalloc(dev->dev, sizeof(*encoder), GFP_KERNEL);
- if (!encoder) return -EINVAL;
- output->dc = dc;
- drm_encoder_helper_add(&output->encoder,
&atmel_hlcdc_panel_encoder_helper_funcs);- ret = drm_encoder_init(dev, &output->encoder,
- ret = drm_encoder_init(dev, encoder, &atmel_hlcdc_panel_encoder_funcs,
atmel_hlcdc_panel_encoder_funcs should probably be renamed atmel_hlcdc_dpi_encoder_funcs, but it's just a detail.
DRM_MODE_ENCODER_NONE, NULL);if (ret) return ret;
- output->encoder.possible_crtcs = 0x1;
encoder->possible_crtcs = 0x1;
ret = drm_of_find_panel_or_bridge(np, 0, 0, &panel, &bridge); if (ret) return ret;
if (panel) {
output->connector.dpms = DRM_MODE_DPMS_OFF;output->connector.polled = DRM_CONNECTOR_POLL_CONNECT;drm_connector_helper_add(&output->connector,&atmel_hlcdc_panel_connector_helper_funcs);ret = drm_connector_init(dev, &output->connector,&atmel_hlcdc_panel_connector_funcs,DRM_MODE_CONNECTOR_Unknown);if (ret)goto err_encoder_cleanup;drm_mode_connector_attach_encoder(&output->connector,&output->encoder);ret = drm_panel_attach(panel, &output->connector);if (ret) {drm_connector_cleanup(&output->connector);goto err_encoder_cleanup;}output->panel = panel;
bridge = drm_panel_bridge_add(panel, DRM_MODE_CONNECTOR_Unknown);if (IS_ERR(bridge))return PTR_ERR(bridge);It would even be simpler if panels were directly registering themselves to the bridge infrastructure so that they can be found with drm_of_find_bridge() and we don't have to explicitly create this bride->panel wrapper, but I remember that Thierry was not a big fan of this idea.
Also, I keep thinking that we're abusing objects. A bridge is supposed to convert a pixel stream in a given format/encoding into a different format/encoding. Here, there's no conversion taking place in this panel_bridge, it's just a way to expose the panel as a generic element that can be easily connected to many display controller drivers. To be perfectly clear, I really like this idea of providing a generic wrapper, I'm just unsure exposing this wrapper as a bridge is such a good idea, unless we clarify the meaning/goal of a 'drm_bridge' object.
The more I look at it, the more I think the drm_bridge is turning into a generic element that is taking a stream of pixel in a specific format and either consuming it of transforming it before forwarding it to the next element in the chain.
Regards,
Boris _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel