Re: [RFC 0/1] drm: Add Grain Media GM12U320 kms driver

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 07-06-17 23:14, Noralf Trønnes wrote:
> 
> Den 07.06.2017 22.19, skrev Noralf Trønnes:
>>
>> Den 07.06.2017 21.50, skrev Marco Diego Aurélio Mesquita:
>>> On Wed, Jun 7, 2017 at 4:38 PM, Noralf Trønnes <noralf@tronnes.org> wrote:
>>>> Den 07.06.2017 20.46, skrev Marco Diego Aurélio Mesquita:
>>>>> Hi Noralf!
>>>>>
>>>>> On Wed, Jun 7, 2017 at 1:56 PM, Noralf Trønnes <noralf@tronnes.org> wrote:
>>>>>> tinydrm is targeted at displays on slow busses where userspace tells
>>>>>> the driver which part of the framebuffer it has changed to minimize
>>>>>> the transfer.
>>>>>>
>>>>>> The PL111 driver uses drm_simple_display_pipe and might be a better
>>>>>> example for your use case:
>>>>>> https://cgit.freedesktop.org/drm/drm-misc/tree/drivers/gpu/drm/pl111
>>>>> I'll investigate how to modify the pl111 driver to fit my needs then.
>>>>> Do you think is there any change that must be done to the
>>>>> pl111_get_panel function regarding the call to
>>>>> of_graph_get_next_endpoint? I mean, the udl driver (which the gm12u320
>>>>> driver is based on) does not have a panel; would any change be needed
>>>>> because of this?
>>>>
>>>> I guess you can keep your get_modes and do something like this:
>>>>
>>>> static const struct drm_connector_helper_funcs gm12u320_helper_funcs = {
>>>>      .get_modes = gm12u320_get_modes,
>>>> };
>>>>
>>>> .best_encoder has a default that fits your 1 encoder case.
>>>>
>>>> static const struct drm_connector_funcs gm12u320_connector_funcs = {
>>>>      .fill_modes = drm_helper_probe_single_connector_modes,
>>>>      .destroy = gm12u320_connector_destroy,
>>>>      .detect = gm12u320_detect,
>>>>      .dpms = drm_atomic_helper_connector_dpms,
>>>>      .reset = drm_atomic_helper_connector_reset,
>>>>      .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
>>>>      .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>>>> };
>>>>
>>>> Then you init and add helper:
>>>>      drm_connector_init(dev, connector, &gm12u320_connector_funcs,
>>>>                 DRM_MODE_CONNECTOR_Unknown);
>>>>      drm_connector_helper_add(connector, &gm12u320_helper_funcs);
>>>>
>>>> And finally attach the connector to the pipe:
>>>>      drm_simple_display_pipe_init(..., connector);
>>>>
>>>> The connector is registered when the drm device is registered.
>>>>
>>> Thanks Noralf! That's exactly what I did and I'll test it soon. The
>>> parts that scare me the most are the calls to
>>> of_graph_get_next_endpoint, of_property_read_u32_array and of_node_put
>>> (in display.c, for example). I'm still not sure those will work.
>>
>> You don't need those since you don't have a panel nor those pads:
>> https://cgit.freedesktop.org/drm/drm-misc/tree/Documentation/devicetree/bindings/display/arm,pl11x.txt
>>
>> The reason I pointed you to the PL111 driver is because it's very simple,
>> so it's easy to see how the simple pipeline works.
>> It uses CMA memory and if you can use that all is well, but if you need
>> to access the framebuffer from the cpu, it's not so good, because cma
>> memory has uncached reads.
>> The udl driver handles memory in a different way.
>>
>> I'm out of my depths here with memory and GEM and stuff.
>>
> 
> I see that you access the framebuffer in gm12u320_copy_fb_to_blocks().
> That will be be very slow with CMA memory. I have worked around this in
> tinydrm by caching one line at a time to speed it up. I don't know if
> this will work for you. tinydrm displays are very small so the overhead
> isn't big compared to the transfer speed.
> See http://elixir.free-electrons.com/linux/latest/ident/tinydrm_swab16
> 
> Maybe you can cache one cacheline size at a time? After all, you have to
> pull it through the cpu cache. But as I said, memory isn't my expertise.
> 
> It greatly simplifies your driver if you can use the cma library though.
> If so, maybe you even can use tinydrm with a connector of your own. You
> can just make your own version of tinydrm_display_pipe_init().

I don't think that using cma for the gm12u320 is a good idea, it will
typically be used as a secondary GPU output together with a real GPU
extending the desktop by being a prime display output. So for the memory
management stuff I would keep the code copied from the udl driver (which
we may later split out in a separate helper lib for devices where
the framebuffer is in normal system memory and we have some scather-gather
capable process copying it to the real device over e.g. USB)

Regards,

Hans
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel