thx Daniel.. I'll shortly be sending an updated patch with some changes based on your comments and some TODO updates..
On Wed, Oct 19, 2011 at 8:27 AM, Daniel Vetter daniel@ffwll.ch wrote:
On Fri, Oct 14, 2011 at 10:45:50AM -0500, Rob Clark wrote:
[snip]
- dev->mode_config.min_width = 256;
- dev->mode_config.min_height = 256;
- /* note: pvr can't currently handle dst surfaces larger than 2k by 2k */
- dev->mode_config.max_width = 2048;
- dev->mode_config.max_height = 2048;
Aside we usually put the limits of the scanout engine here, not the limits of the 3d core. E.g. i915 has 4k scanout limit with a 2k limit for the 2d core, too (for gen3 chipsets).
ok, well 2k is currently also the scanout limit, although in future I'll have to add some omap revision # checks..
[snip]
+static void omap_encoder_dpms(struct drm_encoder *encoder, int mode) +{
- struct omap_encoder *omap_encoder = to_omap_encoder(encoder);
- struct drm_device *dev = encoder->dev;
- struct omap_drm_private *priv = dev->dev_private;
- int i;
- DBG("%s: %d", omap_encoder->mgr->name, mode);
- /* managers don't need to do anything for DPMS.. but we do
- * need to propagate to the connector, who is actually going
- * to enable/disable as needed:
- */
- for (i = 0; i < priv->num_connectors; i++) {
- struct drm_connector *connector = priv->connectors[i];
- if (connector->encoder == encoder) {
- omap_connector_dpms(connector, mode);
- }
- }
+}
I think the dpms helpers are a bad fit for you, and your abusing infrastructure a bit ;-) I think better would be to but omap_connector_dpms into the connector dpms function and maybe call drm_helper_connector_dpms from there, if you still need the outmagic dpms calls on crtcs (with a dummy dpms function on encoders).
Core drm only supports dpms on an connector. The helper function is just for the common case where you only have dpms state on crtcs/encoders and they need to be as active as the most active connector (see the drm_helper_choose_*_dpms functions, too).
ok, I've re-worked this one..
+static bool omap_encoder_mode_fixup(struct drm_encoder *encoder,
- struct drm_display_mode *mode,
- struct drm_display_mode *adjusted_mode)
+{
- struct omap_encoder *omap_encoder = to_omap_encoder(encoder);
- DBG("%s", omap_encoder->mgr->name);
- return true;
+}
+static void omap_encoder_mode_set(struct drm_encoder *encoder,
- struct drm_display_mode *mode,
- struct drm_display_mode *adjusted_mode)
+{
- struct omap_encoder *omap_encoder = to_omap_encoder(encoder);
- struct drm_device *dev = encoder->dev;
- struct omap_drm_private *priv = dev->dev_private;
- int i;
- mode = adjusted_mode;
- DBG("%s: set mode: %dx%d", omap_encoder->mgr->name,
- mode->hdisplay, mode->vdisplay);
- for (i = 0; i < priv->num_connectors; i++) {
- struct drm_connector *connector = priv->connectors[i];
- if (connector->encoder == encoder) {
- omap_connector_mode_set(connector, mode);
- }
- }
This also looks like something the drm helpers should do for you or you're using them in a strange way.
drm core does the modeset on a crtc only, drm_crtc_helper_set_config should then do the right thing of walking over all connectors/encoders calling set_mode and finally the mode_set_base on the crtc (roughly).
I think you need to recheck what stuff you're setting in connectors and what in encoders (or if they are just dummies with a 1:1 connector mapping).
... but this one, I don't see a better way. The problem is that omap_dss_driver is sort of a combination of encoder and connector. So I think this one I live with for now. Long term, once we have drm_plane stuff, I'm not really sure if it is needed to keep separate dss and omapdrm layers. So until then I wasn't really trying to do too much changing of dss APIs.
[snip]
+static void omap_framebuffer_destroy(struct drm_framebuffer *fb) +{
- struct drm_device *dev = fb->dev;
- struct omap_framebuffer *omap_fb = to_omap_framebuffer(fb);
- DBG("destroy: FB ID: %d (%p)", fb->base.id, fb);
- drm_framebuffer_cleanup(fb);
This is a bit a general mess in kms. All drivers need to call this, and for added hilarity
- drm_crtc.c drm_mode_rmfb has a TODO that this is missing
- nouveau/radeon only do this _after_ unref'ing the backing storage
- gma500 also tries to restore the kernel console here which should be
done in lastclose (because you might disable another userspace fb on another output).
Can I prod you to write the patches to clean this up and move drm_framebuffer_cleanup into common code?
Ok, sure.. I'll do this, but as a later patch
BR, -R