| Message ID | 20190207083647.20615-2-linus.walleij@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | DRM driver for ST-Ericsson MCDE | expand |
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 >
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
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
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);
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(-)