Hi Eric,
Thanks for your reply back in April. I have finally found some time to
get back to this.
On 18 April 2018 at 03:43, Eric Anholt <eric@xxxxxxxxxx> wrote:
> Joel Stanley <joel@xxxxxxxxx> writes:
>
>> This driver is for the ASPEED BMC SoC's GFX display hardware. This
>> driver runs on the ARM based BMC systems, unlike the ast driver which
>> runs on a host CPU and is is for a PCIe graphics device that happens to
>> live in the BMC's silicon, but is otherwise available for use by the BMC.
>>
>> The AST2500 supports a total of 3 output paths:
>>
>> 1. VGA output (aka host PCIe graphics device), the output target can
>> choose either or both to the DAC or DVO interface.
>>
>> 2. Graphics CRT output, the output target can choose either or both to
>> the DAC or DVO interface.
>>
>> 3. Video input from DVO, the video input can be used for video engine
>> capture or DAC display output.
>>
>> Output options for are selected in SCU2C. This must be toggled by the
>> BMC in order to select between the host and BMC's display output.
>>
>> The "VGA mode" device is the PCI attached controller. The "Graphics CRT"
>> is the BMC's internal display controller.
>>
>> This driver only supports a simple configuration consisting of a 40MHz
>> pixel clock (fixed by hardware limitations) and the VGA output path.
>
> The confusing part of this driver to me is understanding where this
> display output is going -- if you're going out a VGA connector to a
> monitor (is that what "DAC" meant?), we ought to have
> DRM_MODE_CONNECTOR_VGA and a .get_modes that does I2C to get the EDID.
> If it's fed into the input of some other display controller, then I'm
> not sure what's the right thing to do.
Yes, DAC is what the datasheet calls the VGA connector. The hardware
has optional i2c lines, but they are not hooked up on any of the
systems I have access to.
I think most systems hard code a single mode, and this is the approach
we have taken when integrating this driver into OpenBMC.
>
> I'd recommend moving some of this commit message into kerneldoc in the
> _drv.c explaining what the HW does and what parts of the HW the driver
> exposes.
Good idea.
>
>> Signed-off-by: Joel Stanley <joel@xxxxxxxxx>
>> ---
>>
>> Hello!
>>
>> This driver is working on hardware, with a few oddities. The things that
>> I know need cleaning up are:
>>
>> - clocks: The device can source a clock from three different sources,
>> but they depend on the configuration of other devices in the system
>> (the USB PHY and the 'host' display controller). For this reason we
>> limit the driver to the 40MHz pixel clock that comes from the USB
>> PHY.
>
> You may want to look into setting up a little mux clock provider that
> can source from whatever parents are available to get you the best,
> faster-than-requested clock. Then, if the rate it gives you is too far
> off from what you wanted, maybe stretch the hblank intervals of your
> modeline to get as close to the target vrefresh as possible (check out
> vc4_dsi.c:vc4_dsi_encoder_mode_fixup() for an example of doing this)
>
>> - Setting an initial mode. I can only get the system to output when
>> running X. fbset and fb-test can't seem to do this on their own. The
>> system this is being developed for intends to run a simple fbterm
>> based application, so this needs fixing.
>
> Not sure what would be going on here. You do have all of the DRM FB
> emulation bits enabled, right?
I sorted this out in the end. I had the enable_vblank/disable_vblank
callbacks hooked up to struct drm_driver.
As I was using the drm_simple stuff, it adds it's own enable_vblank on
struct drm_crtc_funcs (which then pokes into struct
drm_simple_display_pipe to grab the real vblank, if it exists).
The fb enable code checks to see if drm_crtc_funcs->enable_vblank
exists, and if it does, calls that and returns without calling the one
on struct drm_driver.
Once I discovered that having enable_vblank on drm_driver is
depreciated, the solution became obvious.
>
>> I wanted to get feedback on the way the driver is structured, and if
>> possible a some advice on why fb tools can't set a mode themselves.
>
> drm_simple_display_pipe seems like a good way to get started.
> Eventually to merge you'll need a DT binding document
> (Documentation/devicetree/bindings/display)
>
>> diff --git a/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c b/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c
>> +#include "aspeed_gfx.h"
>> +
>> +static const struct drm_mode_config_funcs aspeed_gfx_mode_config_funcs = {
>> + .fb_create = drm_gem_fb_create,
>> + .atomic_check = drm_atomic_helper_check,
>> + .atomic_commit = drm_atomic_helper_commit,
>> +};
>
> I wonder if .output_poll_changed = drm_fb_helper_output_poll_changed,
> might help your fbdev setup?
fwiw this did not help.
>
>> +static void aspeed_gfx_setup_mode_config(struct drm_device *drm)
>> +{
>> + drm_mode_config_init(drm);
>> +
>> + drm->mode_config.min_width = 0;
>> + drm->mode_config.min_height = 0;
>> + drm->mode_config.max_width = 800;
>> + drm->mode_config.max_height = 600;
>> + drm->mode_config.funcs = &aspeed_gfx_mode_config_funcs;
>> +}
>> +
>> +static int aspeed_gfx_load(struct drm_device *drm)
>> +{
>> +
>> + ret = drm_irq_install(drm, platform_get_irq(pdev, 0));
>> + if (ret < 0) {
>> + dev_err(drm->dev, "Failed to install IRQ handler\n");
>> + return ret;
>> + }
>> +
>> + drm_mode_config_reset(drm);
>> +
>> + priv->fbdev = drm_fbdev_cma_init(drm, 32, 1);
>> + if (IS_ERR(priv->fbdev)) {
>> + ret = PTR_ERR(priv->fbdev);
>> + dev_err(drm->dev, "Failed to init FB CMA area\n");
>> + goto err_cma;
>> + }
>
> I think you want to use drm_fb_cma_fbdev_init() here, which lets you
> drop your .lastclose. Take a look at commit
> bdecd83546352e0cdf54f64d8d6206f1fef32d75, for example.
Nice.
>
>> +
>> + return 0;
>> +
>> +err_cma:
>> + drm_irq_uninstall(drm);
>> + return ret;
>> +}
>> +
>> +static struct drm_driver aspeed_gfx_driver = {
>> + .driver_features = DRIVER_GEM | DRIVER_MODESET |
>> + DRIVER_PRIME | DRIVER_ATOMIC |
>> + DRIVER_HAVE_IRQ,
>> + .lastclose = aspeed_gfx_lastclose,
>> + .irq_handler = aspeed_gfx_irq_handler,
>> + .irq_preinstall = aspeed_gfx_irq_preinstall,
>> + .irq_uninstall = aspeed_gfx_irq_preinstall,
>
> I suspect that uninstall shouldn't be the same as preinstall :)
>
> Actually, I think the preferred way these days is to not use the old
> core IRQ stuff, and just devm_request_irq(dev, platform_get_irq(pdev,
> 0), ...) in your _drv.c
Sounds good.
>
>> + .enable_vblank = aspeed_gfx_enable_vblank,
>> + .disable_vblank = aspeed_gfx_disable_vblank,
>> + .gem_free_object_unlocked = drm_gem_cma_free_object,
>> + .gem_vm_ops = &drm_gem_cma_vm_ops,
>> + .dumb_create = drm_gem_cma_dumb_create,
>> + .prime_handle_to_fd = drm_gem_prime_handle_to_fd,
>> + .prime_fd_to_handle = drm_gem_prime_fd_to_handle,
>> + .gem_prime_export = drm_gem_prime_export,
>> + .gem_prime_import = drm_gem_prime_import,
>> + .gem_prime_get_sg_table = drm_gem_cma_prime_get_sg_table,
>> + .gem_prime_import_sg_table = drm_gem_cma_prime_import_sg_table,
>> + .gem_prime_vmap = drm_gem_cma_prime_vmap,
>> + .gem_prime_vunmap = drm_gem_cma_prime_vunmap,
>> + .gem_prime_mmap = drm_gem_cma_prime_mmap,
>> + .fops = &fops,
>> + .name = "aspeed-gfx-drm",
>> + .desc = "ASPEED GFX DRM",
>> + .date = "20180319",
>> + .major = 1,
>> + .minor = 0,
>> +};
>> +
>> diff --git a/drivers/gpu/drm/aspeed/aspeed_gfx_out.c b/drivers/gpu/drm/aspeed/aspeed_gfx_out.c
>> +int aspeed_gfx_create_output(struct drm_device *drm)
>> +{
>> + struct aspeed_gfx *priv = drm->dev_private;
>> + int ret;
>> +
>> + priv->connector.dpms = DRM_MODE_DPMS_OFF;
>
> I see this DPMS line present in other drivers, but I'm skeptical of it.
I'll test without this line.
Thanks for the review!
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
