[PATCH] DRM: omapdrm DRM/KMS driver for TI OMAP platforms

[Rob Clark]

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 at 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