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

[Sean Paul]

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).

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