On Fri, 2019-06-14 at 13:05 -0700, Doug Anderson wrote:
> Hi,
>
> On Thu, Jun 13, 2019 at 12:23 PM Ezequiel Garcia <ezequiel@xxxxxxxxxxxxx> wrote:
> > @@ -1744,6 +1793,41 @@ int rockchip_drm_wait_vact_end(struct drm_crtc *crtc, unsigned int mstimeout)
> > }
> > EXPORT_SYMBOL(rockchip_drm_wait_vact_end);
> >
> > +static int vop_gamma_lut_request(struct device *dev,
> > + struct resource *res, struct vop *vop)
> > +{
> > + resource_size_t offset = vop->data->gamma_lut_addr_off;
> > + resource_size_t size = VOP_GAMMA_LUT_SIZE * 4;
> > +
> > + /*
> > + * Some SoCs (e.g. RK3288) have the gamma LUT address after
> > + * the MMU registers, which means we can't request and ioremap
> > + * the entire register set. Other (e.g. RK3399) have gamma LUT
> > + * address before MMU.
> > + *
> > + * Therefore, we need to request and ioremap those that haven't
> > + * been already.
> > + */
> > + if (vop->len >= (offset + size)) {
> > + vop->lut_regs = vop->regs + offset;
> > + return 0;
> > + }
> > +
> > + if (!devm_request_mem_region(dev, res->start + offset,
> > + size, dev_name(dev))) {
> > + dev_warn(dev, "can't request gamma lut region\n");
> > + return -EBUSY;
> > + }
> > +
> > + vop->lut_regs = devm_ioremap(dev, res->start + offset, size);
> > + if (!vop->lut_regs) {
> > + dev_err(dev, "can't ioremap gamma lut address\n");
> > + devm_release_mem_region(dev, res->start + offset, size);
> > + return -ENOMEM;
> > + }
>
> I'm curious here. I was always under the impression that you were
> supposed to specify all of your memory regions in the device tree.
> ...but here the device tree on rk3288 says:
>
> vopb: vop@ff930000 {
> compatible = "rockchip,rk3288-vop";
> reg = <0x0 0xff930000 0x0 0x19c>;
> ...
> };
>
> ...and we're now mapping 4096 bytes starting at 0xff931000. Is that
> really legit? Wouldn't it be better to put this extra memory range in
> the dts?
>
> Hrm, but then I guess you need to figure out what to do about older
> device trees. Do you disable the gamma LUT feature? ...or do you do
> exactly what the code here is doing and just map it anyway? I guess
> you could just keep the code here (and it'll work fine), but maybe in
> parallel we should add it to the .dts file and bindings?
>
Maybe we can see how it would look adding the LUT as a separate
(optional) resource in the devicetree, and dropping support for RK3399,
which doesn't seem to work.
I'm taking a look at Jacopo's question on atomic_flush vs.
atomic_commit_tail, and will prepare a v2.
> ---
>
> I will say that, though I don't know much (anything?) about gamma
> LUTs, I ran the Chrome OS "gamma_test" program and saw a pretty RGB
> gradient on the both the internal screen and HDMI monitor on my
> rk3288-veyron-jerry. Thus:
>
> Tested-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
Thanks,
Ezequiel
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
