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

[Sean Paul]

On Mon, Oct 21, 2013 at 10:46 AM, Sean Paul <seanpaul at chromium.org> wrote:
> On Thu, Oct 17, 2013 at 10:31 PM, Inki Dae <inki.dae at samsung.com> wrote:
>>
>>
>>> -----Original Message-----
>>> From: Sean Paul [mailto:seanpaul at chromium.org]
>>> Sent: Thursday, October 17, 2013 11:37 PM
>>> To: Inki Dae
>>> Cc: dri-devel; Dave Airlie; Tomasz Figa; Stéphane Marchesin
>>> Subject: Re: [PATCH v2 12/26] drm/exynos: Split manager/display/subdrv
>>>
>>> On Thu, Oct 17, 2013 at 4:21 AM, Inki Dae <inki.dae at samsung.com> wrote:
>>> >
>>> >
>>> >> -----Original Message-----
>>> >> From: Sean Paul [mailto:seanpaul at chromium.org]
>>> >> Sent: Thursday, October 17, 2013 4:27 AM
>>> >> To: dri-devel at lists.freedesktop.org; inki.dae at samsung.com
>>> >> Cc: airlied at linux.ie; tomasz.figa at gmail.com; marcheu at chromium.org; Sean
>>> >> Paul
>>> >> Subject: [PATCH v2 12/26] drm/exynos: Split manager/display/subdrv
>>> >>
>>> >> This patch splits display and manager from subdrv. The result is that
>>> >> crtc functions can directly call into manager callbacks and encoder
>>> >> functions can directly call into display callbacks. This will allow
>>> >> us to remove the exynos_drm_hdmi shim and support mixer/hdmi & fimd/dp
>>> >> with common code.
>>> >>
>>> >> Signed-off-by: Sean Paul <seanpaul at chromium.org>
>>> >> ---
>>> >>
>>> >> Changes in v2:
>>> >>       - Pass display into display_ops instead of context
>>> >
>>> > Sorry but it seems like more reasonable to pass device object into
>>> > display_ops and manager_ops.
>>> >
>>>
>>>
>>> So you've changed your mind from when you said the following?
>>>
>>> >>> manager->ops->xxx(manager, ...);
>>> >>> display->ops->xxx(display, ...);
>>> >>>
>>> >>> Agree.
>>>
>>
>>
>> True. Before that, My comment was to pass device object into display_ops and
>> manager_ops, and then you said the good solution is to pass manager and
>> display to each driver. At that time, I thought no matter how the callback
>> is called if the framework doesn't call callbacks of each driver with ctx.
>> So I agreed.
>>
>>
>>> It would have been nice if you had changed your mind *before* I
>>> reworked everything. At any rate, I think it's still the right thing
>>> to do.
>>
>> Really sorry about that. And I will add new patch for it so you don't need
>> to concern about that.
>>
>>>
>>>
>>> > I'm not sure but display_ops could be implemented in other framework
>>> based
>>> > driver such as CDF based lcd panel driver. So if you pass display - it's
>>> > specific to exynos drm framework - into display_ops, the other framework
>>> > based driver should include specific exynos drm header.
>>> >
>>>
>>> AFAIK, CDF will not land in its current separate-from-drm form, we
>>> don't need to worry about this. Furthermore, these ops should just go
>>> away and become drm_crtc/drm_encoder/drm_connector funcs anyways.
>>>
>>
>> Can you assure the display_ops never implemented in other framework based
>> driver, not CDF? At any rate, I think all possibilities should be opened.
>>
>
> I don't think we should let an RFC framework make the code more
> complicated for unclear benefit. By removing manager/display entirely,
> we can get rid of a *lot* of other code that is basically just
> plumbing drm hooks (exynos_drm_connector is a good example).
>

I hacked this up today to prove it out. Check out the top 5 commits in
https://github.com/crseanpaul/exynos-drm-next/commits/linux-next-exynos-staging.
It removes exynos_drm_connector in favor of just implementing
drm_connector directly. This same treatment should be done for
exynos_drm_encoder and exynos_drm_crtc, but I didn't get around to
doing this.

As you can see, it cuts out a lot of code and removes an entire
abstraction layer. Much nicer :)

Sean

>>>
>>> > And another one, the patch 6 passes manager object to manager_ops, and
>>> for
>>> > this, you made the manager object to be set to driver data;
>>> > platform_set_drvdata(pdev, &manager). That isn't reasonable. Generally,
>>> > driver_data would point to device driver's context object.
>>> >
>>>
>>> I'm not sure why this isn't reasonable, but it's a moot point. The
>>> driver data is only used up until we get rid of the pm ops, it needn't
>>> be set at all once things go through dpms.
>>>
>>
>> Generally, device drivers can call its own internal functions, and they will
>> call that functions with ctx. However, if you set manager to driver_data
>> then that functions should be called with manager object and also internally
>> that functions should get ctx from the manager object. What is the purpose
>> of manager? Do you think it's reasonable?
>>
>
> So, to avoid setting the manager as the drvdata, we could implement
> something like fimd_dpms_ctx(ctx, mode) that takes ctx and the manager
> callback calls it fimd_dpms(mgr, mode) { ctx = mgr->ctx;
> fimd_dpms_ctx(ctx, mode); }. Alternatively, you can save a pointer to
> mgr in ctx, but that creates a circular link between the two. IMO,
> both of those solutions suck :)
>
> I'd much rather just set drvdata to the manager and call the hook
> directly. Like I said earlier, this is just a temporary step since we
> remove these pm ops later in the patch series.
>
> Sean
>
>
>> Anyway, I'd like to say really sorry about inconvenient again. So I will fix
>> it.
>>
>> Thanks,
>> Inki Dae
>>
>>> Sean
>>>
>>>
>>> > Thanks,
>>> > Inki Dae
>>> >
>>