[1/4] drm/simple_kms_helper: enable use of external encoder
diff mbox series

Message ID 20190207083647.20615-2-linus.walleij@linaro.org
State New
Headers show
Series
  • DRM driver for ST-Ericsson MCDE
Related show

Commit Message

Linus Walleij Feb. 7, 2019, 8:36 a.m. UTC
This makes it possible to pass a connector with an already
attached external encoder into the simple KMS helper.

This is helpful for my MCDE drivers, as it is pretty simple
but uses DSI to communicate with the displays and bridges.
DSI requires the use of the DSI bus which in turn requires
us to set up a custom connector from the display driver.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpu/drm/drm_simple_kms_helper.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

Comments

Daniel Vetter Feb. 7, 2019, 9:17 a.m. UTC | #1
On Thu, Feb 07, 2019 at 09:36:44AM +0100, Linus Walleij wrote:
> This makes it possible to pass a connector with an already
> attached external encoder into the simple KMS helper.
> 
> This is helpful for my MCDE drivers, as it is pretty simple
> but uses DSI to communicate with the displays and bridges.
> DSI requires the use of the DSI bus which in turn requires
> us to set up a custom connector from the display driver.

So the idea was that you'd just use a bridge for this, if you need more
than a dummy encoder. I'm a bit worried about mission creep for the simple
helpers, sooner or later we'll add extensions and then we're back to full
kms, except it's a hairball of classic midlayer mistake ...

See drm_simple_display_pipe_attach_bridge().

Cheers, Daniel

> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/gpu/drm/drm_simple_kms_helper.c | 23 ++++++++++++++++++-----
>  1 file changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c
> index 917812448d1b..e7499b939235 100644
> --- a/drivers/gpu/drm/drm_simple_kms_helper.c
> +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
> @@ -266,7 +266,7 @@ int drm_simple_display_pipe_init(struct drm_device *dev,
>  			const uint64_t *format_modifiers,
>  			struct drm_connector *connector)
>  {
> -	struct drm_encoder *encoder = &pipe->encoder;
> +	struct drm_encoder *encoder;
>  	struct drm_plane *plane = &pipe->plane;
>  	struct drm_crtc *crtc = &pipe->crtc;
>  	int ret;
> @@ -289,10 +289,23 @@ int drm_simple_display_pipe_init(struct drm_device *dev,
>  	if (ret)
>  		return ret;
>  
> -	encoder->possible_crtcs = drm_crtc_mask(crtc);
> -	ret = drm_encoder_init(dev, encoder, &drm_simple_kms_encoder_funcs,
> -			       DRM_MODE_ENCODER_NONE, NULL);
> -	if (ret || !connector)
> +	/* Other encoder already attached to the connector */
> +	if (connector->encoder_ids[0] != 0) {
> +		encoder = drm_encoder_find(connector->dev, NULL,
> +					   connector->encoder_ids[0]);
> +		encoder->possible_crtcs = drm_crtc_mask(crtc);
> +		DRM_INFO("an encoder is already attached to the connector\n");
> +	} else {
> +		encoder = &pipe->encoder;
> +		encoder->possible_crtcs = drm_crtc_mask(crtc);
> +		ret = drm_encoder_init(dev, encoder,
> +				       &drm_simple_kms_encoder_funcs,
> +				       DRM_MODE_ENCODER_NONE, NULL);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (!connector)
>  		return ret;
>  
>  	return drm_connector_attach_encoder(connector, encoder);
> -- 
> 2.20.1
>
Linus Walleij Feb. 7, 2019, 9:02 p.m. UTC | #2
On Thu, Feb 7, 2019 at 10:17 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> On Thu, Feb 07, 2019 at 09:36:44AM +0100, Linus Walleij wrote:

> > This makes it possible to pass a connector with an already
> > attached external encoder into the simple KMS helper.
> >
> > This is helpful for my MCDE drivers, as it is pretty simple
> > but uses DSI to communicate with the displays and bridges.
> > DSI requires the use of the DSI bus which in turn requires
> > us to set up a custom connector from the display driver.
>
> So the idea was that you'd just use a bridge for this, if you need more
> than a dummy encoder. I'm a bit worried about mission creep for the simple
> helpers, sooner or later we'll add extensions and then we're back to full
> kms, except it's a hairball of classic midlayer mistake ...
>
> See drm_simple_display_pipe_attach_bridge().

Attaching bridges usually work fine for simple KMS drivers,
like the panel bridge, or the dumb VGA bridge or even
the HDMI encoder bridges so I'm on board with that.

The thing is that the DSI driver is using the panel bridge
(and has a placeholder for using other bridges) already.
So we end up wrapping the DSI host device(s) with a
"my DSI bridge" using another
bridge (panel) IIUC.

display driver -> custom DSI bridge -> panel bridge -> panel

The endpoint bridge (the panel) has all information on
resolution etc. So this means we will need to just pass this
information through to the next step in the pipe I
suppose.

I just want to confirm that I'm on the right track here before
I code another thousand lines of code for this :)

Yours,
Linus Walleij
Daniel Vetter Feb. 7, 2019, 9:43 p.m. UTC | #3
On Thu, Feb 07, 2019 at 10:02:17PM +0100, Linus Walleij wrote:
> On Thu, Feb 7, 2019 at 10:17 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Thu, Feb 07, 2019 at 09:36:44AM +0100, Linus Walleij wrote:
> 
> > > This makes it possible to pass a connector with an already
> > > attached external encoder into the simple KMS helper.
> > >
> > > This is helpful for my MCDE drivers, as it is pretty simple
> > > but uses DSI to communicate with the displays and bridges.
> > > DSI requires the use of the DSI bus which in turn requires
> > > us to set up a custom connector from the display driver.
> >
> > So the idea was that you'd just use a bridge for this, if you need more
> > than a dummy encoder. I'm a bit worried about mission creep for the simple
> > helpers, sooner or later we'll add extensions and then we're back to full
> > kms, except it's a hairball of classic midlayer mistake ...
> >
> > See drm_simple_display_pipe_attach_bridge().
> 
> Attaching bridges usually work fine for simple KMS drivers,
> like the panel bridge, or the dumb VGA bridge or even
> the HDMI encoder bridges so I'm on board with that.
> 
> The thing is that the DSI driver is using the panel bridge
> (and has a placeholder for using other bridges) already.
> So we end up wrapping the DSI host device(s) with a
> "my DSI bridge" using another
> bridge (panel) IIUC.
> 
> display driver -> custom DSI bridge -> panel bridge -> panel
> 
> The endpoint bridge (the panel) has all information on
> resolution etc. So this means we will need to just pass this
> information through to the next step in the pipe I
> suppose.

Hm, if you have you're mode_check functions working correctly (i.e. make
sure the mode userspace asks for is the one the panel ones), then the mode
will be in the crtc_state->requested_mode that the atomic helpers already
hand to all the bridges. Or well would, if bridge wouldn't have predated
atomic, so this is all a bit a more a mess than strictly needed. But you
should be getting the right mode already.

> I just want to confirm that I'm on the right track here before
> I code another thousand lines of code for this :)

bridges are supposed to be stackable, but I think you're the first to find
out whether that actually works. Afaiui the real stacking fun is if you
need to have some state between the bridges, because you e.g. have a
scaler or transcoder or something like that in-between.

Looking at your patch, converting the encoder into a bridge, and chaining
up the bridges in the right order, is all that should be needed really.
Not retyping the 1k lines of code that actually does stuff.
-Daniel

Patch
diff mbox series

diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c
index 917812448d1b..e7499b939235 100644
--- a/drivers/gpu/drm/drm_simple_kms_helper.c
+++ b/drivers/gpu/drm/drm_simple_kms_helper.c
@@ -266,7 +266,7 @@  int drm_simple_display_pipe_init(struct drm_device *dev,
 			const uint64_t *format_modifiers,
 			struct drm_connector *connector)
 {
-	struct drm_encoder *encoder = &pipe->encoder;
+	struct drm_encoder *encoder;
 	struct drm_plane *plane = &pipe->plane;
 	struct drm_crtc *crtc = &pipe->crtc;
 	int ret;
@@ -289,10 +289,23 @@  int drm_simple_display_pipe_init(struct drm_device *dev,
 	if (ret)
 		return ret;
 
-	encoder->possible_crtcs = drm_crtc_mask(crtc);
-	ret = drm_encoder_init(dev, encoder, &drm_simple_kms_encoder_funcs,
-			       DRM_MODE_ENCODER_NONE, NULL);
-	if (ret || !connector)
+	/* Other encoder already attached to the connector */
+	if (connector->encoder_ids[0] != 0) {
+		encoder = drm_encoder_find(connector->dev, NULL,
+					   connector->encoder_ids[0]);
+		encoder->possible_crtcs = drm_crtc_mask(crtc);
+		DRM_INFO("an encoder is already attached to the connector\n");
+	} else {
+		encoder = &pipe->encoder;
+		encoder->possible_crtcs = drm_crtc_mask(crtc);
+		ret = drm_encoder_init(dev, encoder,
+				       &drm_simple_kms_encoder_funcs,
+				       DRM_MODE_ENCODER_NONE, NULL);
+		if (ret)
+			return ret;
+	}
+
+	if (!connector)
 		return ret;
 
 	return drm_connector_attach_encoder(connector, encoder);