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

[Sean Paul]

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


>>
>> 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;
>> >>  }
>> >>
>