On Tue, Mar 07, 2023 at 11:21:16PM +0100, Heiko Stübner wrote:
> Hi Sascha,
>
> Am Donnerstag, 16. Februar 2023, 11:24:44 CET schrieb Sascha Hauer:
> > The different VOP variants support different maximum resolutions. Reject
> > resolutions that are not supported by a specific variant.
> >
> > This hasn't been a problem in the upstream driver so far as 1920x1080
> > has been the maximum resolution supported by the HDMI driver and that
> > resolution is supported by all VOP variants. Now with higher resolutions
> > supported in the HDMI driver we have to limit the resolutions to the
> > ones supported by the VOP.
> >
> > The actual maximum resolutions are taken from the Rockchip downstream
> > Kernel.
> >
> > Signed-off-by: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
> > ---
> >
> > Notes:
> > Changes since v5:
> > - fix wrong check height vs. width
> >
> > Changes since v4:
> > - Use struct vop_rect for storing resolution
> >
> > Changes since v3:
> > - new patch
> >
> > drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 15 +++++++++++++++
> > drivers/gpu/drm/rockchip/rockchip_drm_vop.h | 6 ++++++
> > drivers/gpu/drm/rockchip/rockchip_drm_vop2.h | 5 -----
> > drivers/gpu/drm/rockchip/rockchip_vop_reg.c | 18 ++++++++++++++++++
> > 4 files changed, 39 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> > index fa1f4ee6d1950..40c688529d44e 100644
> > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> > @@ -1174,6 +1174,20 @@ static void vop_crtc_disable_vblank(struct drm_crtc *crtc)
> > spin_unlock_irqrestore(&vop->irq_lock, flags);
> > }
> >
> > +static enum drm_mode_status vop_crtc_mode_valid(struct drm_crtc *crtc,
> > + const struct drm_display_mode *mode)
> > +{
> > + struct vop *vop = to_vop(crtc);
> > +
> > + if (vop->data->max_output.width && mode->hdisplay > vop->data->max_output.width)
> > + return MODE_BAD_HVALUE;
> > +
> > + if (vop->data->max_output.height && mode->vdisplay > vop->data->max_output.height)
> > + return MODE_BAD_VVALUE;
> > +
> > + return MODE_OK;
> > +}
>
> I'm very much in favor of codifying the possible resolutions. Hopefully
> this will also enable better vop-selection down the road.
>
> But ...
>
> The above does break the px30-minievb display.
> While the px30 TRM does say it supports a 1920x1080 resolution only, the
> px30-minievb comes with a 720x1280 DSI display and normally runs just
> fine with it.
>
> Looking at the vendor-code [0], it seems they only seem to check for the
> hvalue. Looking deeper, the height-check was present in the beginning [1],
> but then was removed later on.
>
> Looking a bit more, I find [2] which says that
> "Actually vop hardware has no output height limit"
>
> I re-checked this on both px30+dsi and rock64+1080p-hdmi and with
> > + if (vop->data->max_output.height && mode->vdisplay > vop->data->max_output.height)
> > + return MODE_BAD_VVALUE;
> line gone, rock64 is still happy and the px30 works correctly again.
>
> So, do you see an issue with removing the output-height check?
I just added the height checks for the sake of completeness. For what I
am trying to achieve the width check is sufficient. If there is no
height limitation in hardware anyway then we should just drop the check.
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
