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@xxxxxxxx> 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
