Re: [PATCH v6 1/4] drm/rockchip: vop: limit maximium resolution to hardware capabilities

[Sascha Hauer <s.hauer@pengutronix.de>]

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@pengutronix.de>
> > ---
> > 
> > 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 |

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip