[PATCH 12/23] drm/exynos: Split manager/display/subdrv

[Inki Dae]

> -----Original Message-----
> From: Sean Paul [mailto:seanpaul at chromium.org]
> Sent: Saturday, October 12, 2013 1:20 PM
> To: Inki Dae
> Cc: DRI mailing list; Stéphane Marchesin; Dave Airlie; Tomasz Figa
> Subject: Re: [PATCH 12/23] drm/exynos: Split manager/display/subdrv
> 
> On Fri, Oct 11, 2013 at 11:49 PM, Inki Dae <inki.dae at samsung.com> wrote:
> >
> >
> >> -----Original Message-----
> >> From: Sean Paul [mailto:seanpaul at chromium.org]
> >> Sent: Saturday, October 12, 2013 11:07 AM
> >> To: Inki Dae
> >> Cc: DRI mailing list; Stéphane Marchesin; Dave Airlie; Tomasz Figa
> >> Subject: Re: [PATCH 12/23] drm/exynos: Split manager/display/subdrv
> >>
> >> On Fri, Oct 11, 2013 at 8:42 PM, Inki Dae <inki.dae at samsung.com> wrote:
> >> >>  static int exynos_drm_create_enc_conn(struct drm_device *dev,
> >> >> -                                       struct exynos_drm_subdrv
> > *subdrv)
> >> >> +                                       struct exynos_drm_display
> > *display)
> >> >>  {
> >> >>         struct drm_encoder *encoder;
> >> >>         struct drm_connector *connector;
> >> >> +       struct exynos_drm_manager *manager;
> >> >>         int ret;
> >> >> +       unsigned long possible_crtcs = 0;
> >> >>
> >> >> -       subdrv->manager->dev = subdrv->dev;
> >> >> +       /* Find possible crtcs for this display */
> >> >> +       list_for_each_entry(manager, &exynos_drm_manager_list, list)
> >> >> +               if (manager->type == display->type)
> >> >> +                       possible_crtcs |= 1 << manager->pipe;
> >> >>
> >> >>         /* create and initialize a encoder for this sub driver. */
> >> >> -       encoder = exynos_drm_encoder_create(dev, subdrv->manager,
> >> >> -                       (1 << MAX_CRTC) - 1);
> >> >> +       encoder = exynos_drm_encoder_create(dev, display,
> >> possible_crtcs);
> >> >>         if (!encoder) {
> >> >>                 DRM_ERROR("failed to create encoder\n");
> >> >>                 return -EFAULT;
> >> >>         }
> >> >> -       subdrv->encoder = encoder;
> >> >> +       display->encoder = encoder;
> >> >>
> >> >> -       if (subdrv->manager->display_ops->type ==
> >> EXYNOS_DISPLAY_TYPE_LCD) {
> >> >> +       if (display->type == EXYNOS_DISPLAY_TYPE_LCD) {
> >> >>                 ret = exynos_drm_attach_lcd_bridge(dev, encoder)
> >> >
> >> > Plz, _do_not_base_ on top of your lvds bridge patch set. As I
> >> > mentioned before, the tricky codes are not good. For this, let's find
> >> > a better way after your refactoring patch set are reviewed enough,
> and
> >> > if merged.
> >> >
> >>
> >> [+adding back the people you removed from CC]
> >>
> >
> > Hm.. I  didn't really remove the people from CC. See the your original
> > email. :)
> >
> >
> >> I'm happy to change how that works, but I've yet to find a better
> >> solution. Instead of just dismissing something as "tricky codes", can
> >> you suggest a concrete solution that is better than what I've got?
> >>
> >
> > I think I mentioned already at previous email. First of all, let's have
> a
> > thorough review to your refactoring patch set, and then let's find a
> better
> > way.
> >
> > I believe that we can find a better way with your refactoring patch set:
> now
> > we can implement encoder and connector directly.
> >
> 
> The code will look the exact same, it'll just be in the dp driver
> instead of drm_core (since drm_core will not create
> encoders/connectors). Adding the ptn driver in the interim allows me
> to boot exynos5250-snow with linux-next, so I can test my changes.
> 
> 
> >
> >> I'm also a bit confused as to why you think it's so tricky, it's a
> >
> > First, That was your comment, "Right, so this is kind of tricky". And
> also
> > it seems like tricky codes to me.
> >
> 
> Right, it was a tricky problem (for reasons I've stated multiple
> times), but the solution is dead easy to understand.
> 
> 
> >> synchronous initialization call to a driver; that's about as simple as
> >> it gets.
> >
> > As you mentioned before, ptn3460 driver is not depended on SoC, and can
> be
> > used for other SoC. Let you try to think about lcd class and backlight
> > class. I think we need a interfacing layer between common driver and
> > specific something for connecting SoC specific DRM framework.
> >
> 
> Laurent mentioned that he wanted to come up with a way to model
> graphics hardware in dt. He could then build a notifier when the
> various pieces were successfully probed, and drm would only load once
> all of the pieces were ready.
> 
> I think that would be a nice long-term solution, however let's not let
> the perfect be the enemy of the good. If you can simplify how this
> works, I'm all for it. However, I don't see any other way at the
> moment and I don't think it's productive to just wait around for
> something better to come along.
> 

Sean, let's have reviews to your refactoring patch set first. And then I
will merge your lvds bridge patch if we cannot find a better way. That would
not take much time.

> Sean
> 
> 
> >>
> >> Sean
> >>
> >>
> >>
> >> >>                 if (ret)
> >> >>                         return 0;
> >> >> @@ -91,7 +98,7 @@ static int exynos_drm_create_enc_conn(struct
> >> drm_device *dev,
> >> >>                 goto err_destroy_encoder;
> >> >>         }
> >> >>
> >> >> -       subdrv->connector = connector;
> >> >> +       display->connector = connector;
> >> >>
> >> >>         return 0;
> >> >>
> >> >> @@ -100,21 +107,6 @@ err_destroy_encoder:
> >> >>         return ret;
> >> >>  }
> >> >>
> >