Hi Laurent,
I missed your response. Sorry for that.
> From: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> Date: Mon, Oct 7, 2024 at 7:34 PM
> Subject: Re: [PATCH] drm: renesas: rz-du: rzg2l_du_crtc: Fix max dot clock for DPI
>
>
> Hi Biju,
>
> Thank you for the patch.
>
> On Sat, Oct 05, 2024 at 04:37:31PM +0100, Biju Das wrote:
> > As per the RZ/G2UL hardware manual Table 33.4 Clock List, the maximum
> > dot clock for the DPI interface is 83.5 MHz. Add mode_valid callback
> > to reject modes greater than 83.5 MHz.
> >
> > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > ---
> > Before applying the patch:
> > root@smarc-rzg2ul:~# modetest -M rzg2l-du
> > Encoders:
> > id crtc type possible crtcs possible clones
> > 37 36 none 0x00000001 0x00000001
> >
> > Connectors:
> > id encoder status name size (mm) modes encoders
> > 38 37 connected HDMI-A-1 520x320 30 37
> > modes:
> > index name refresh (Hz) hdisp hss hse htot vdisp vss vse vtot)
> > #0 1920x1080 59.72 1920 1968 2000 2080 1080 1082 1087 1111 138000 flags: phsync, nvsync; type:
> preferred, driver
> > #1 1920x1080 60.00 1920 2008 2052 2200 1080 1084 1089 1125 148500 flags: phsync, pvsync; type:
> driver
> > #2 1920x1080 59.94 1920 2008 2052 2200 1080 1084 1089 1125 148352 flags: phsync, pvsync; type:
> driver
> > #3 1920x1080 59.94 1920 2008 2052 2200 1080 1084 1089 1125 148352 flags: phsync, pvsync; type:
> driver
> > #4 1280x1024 75.02 1280 1296 1440 1688 1024 1025 1028 1066 135000 flags: phsync, pvsync; type:
> driver
> > #5 1280x1024 60.02 1280 1328 1440 1688 1024 1025 1028 1066 108000 flags: phsync, pvsync; type:
> driver
> > #6 1152x864 75.00 1152 1216 1344 1600 864 865 868 900 108000 flags: phsync, pvsync; type: driver
> > #7 1280x720 60.00 1280 1390 1430 1650 720 725 730 750 74250 flags: phsync, pvsync; type: userdef,
> driver
> > #8 1280x720 59.94 1280 1390 1430 1650 720 725 730 750 74176 flags: phsync, pvsync; type: driver
> > #9 1280x720 50.00 1280 1720 1760 1980 720 725 730 750 74250 flags: phsync, pvsync; type: driver
> > ...
> > After applying the patch:
> > root@smarc-rzg2ul:~# modetest -M rzg2l-du
> > Encoders:
> > id crtc type possible crtcs possible clones
> > 37 36 none 0x00000001 0x00000001
> >
> > Connectors:
> > id encoder status name size (mm) modes encoders
> > 38 37 connected HDMI-A-1 520x320 23 37
> > modes:
> > index name refresh (Hz) hdisp hss hse htot vdisp vss vse vtot)
> > #0 1280x720 60.00 1280 1390 1430 1650 720 725 730 750 74250 flags: phsync, pvsync; type: driver
> > #1 1280x720 59.94 1280 1390 1430 1650 720 725 730 750 74176 flags: phsync, pvsync; type: driver
> > #2 1280x720 50.00 1280 1720 1760 1980 720 725 730 750 74250 flags: phsync, pvsync; type: driver
> > #3 1280x720 50.00 1280 1720 1760 1980 720 725 730 750 74250 flags: phsync, pvsync; type: driver
> > #4 1024x768 75.03 1024 1040 1136 1312 768 769 772 800 78750 flags: phsync, pvsync; type: driver
> > #5 1024x768 70.07 1024 1048 1184 1328 768 771 777 806 75000 flags: nhsync, nvsync; type: driver
> > #6 1024x768 60.00 1024 1048 1184 1344 768 771 777 806 65000 flags: nhsync, nvsync; type: driver
> > #7 1080x607 59.97 1080 1120 1232 1384 607 608 611 629 52210 flags: nhsync, pvsync; type:
> > #8 832x624 74.55 832 864 928 1152 624 625 628 667 57284 flags: nhsync, nvsync; type: driver
> > #9 800x600 75.00 800 816 896 1056 600 601 604 625 49500 flags: phsync, pvsync; type: driver
> > ...
> > ---
> > drivers/gpu/drm/renesas/rz-du/rzg2l_du_crtc.c | 43
> > +++++++++++++++++++
> > 1 file changed, 43 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/renesas/rz-du/rzg2l_du_crtc.c
> > b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_crtc.c
> > index 6e7aac6219be..650a2e40caf5 100644
> > --- a/drivers/gpu/drm/renesas/rz-du/rzg2l_du_crtc.c
> > +++ b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_crtc.c
> > @@ -248,6 +248,32 @@ static void rzg2l_du_crtc_stop(struct rzg2l_du_crtc *rcrtc)
> > * CRTC Functions
> > */
> >
> > +static int rzg2l_du_crtc_atomic_check(struct drm_crtc *crtc,
> > + struct drm_atomic_state *state) {
> > + struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state,
> > + crtc);
> > + struct rzg2l_du_crtc_state *rstate = to_rzg2l_crtc_state(crtc_state);
> > + struct drm_encoder *encoder;
> > +
> > + /* Store the routes from the CRTC output to the DU outputs. */
> > + rstate->outputs = 0;
> > +
> > + drm_for_each_encoder_mask(encoder, crtc->dev,
> > + crtc_state->encoder_mask) {
> > + struct rzg2l_du_encoder *renc;
> > +
> > + /* Skip the writeback encoder. */
> > + if (encoder->encoder_type == DRM_MODE_ENCODER_VIRTUAL)
> > + continue;
> > +
> > + renc = to_rzg2l_encoder(encoder);
> > + rstate->outputs |= BIT(renc->output);
> > + }
> > +
> > + return 0;
> > +}
>
> rstate->outputs is used in rzg2l_du_start_stop() but is never
> initialized. It looks like adding this function is a bug fix by itself, I'd split it to a separate
> patch.
Basically this is not required, because of [1] and after implementing .mode_valid() of drm_encoder.
[1] https://patchwork.kernel.org/project/linux-renesas-soc/patch/20241001110141.94504-1-biju.das.jz@xxxxxxxxxxxxxx/
>
> > +
> > static void rzg2l_du_crtc_atomic_enable(struct drm_crtc *crtc,
> > struct drm_atomic_state *state)
> > { @@ -295,10 +321,27 @@ static void rzg2l_du_crtc_atomic_flush(struct
> > drm_crtc *crtc,
> > rzg2l_du_vsp_atomic_flush(rcrtc); }
> >
> > +static enum drm_mode_status
> > +rzg2l_du_crtc_mode_valid(struct drm_crtc *crtc,
> > + const struct drm_display_mode *mode) {
> > + struct rzg2l_du_crtc *rcrtc = to_rzg2l_crtc(crtc);
> > + struct rzg2l_du_crtc_state *rstate =
> > +
> > +to_rzg2l_crtc_state(rcrtc->crtc.state);
>
> I'm not sure that's the right state. When .move_valid() is called from mode_valid_path(), the state
> you need is I believe the new state, not the current state. The issue is that you don't have access to
> the new state here.
>
> Wouldn't it be simpler to implement the .mode_valid() function of drm_encoder ? Then you could check
> renc->output, you wouldn't have to deal with the state.
Agreed, will implement the .mode_valid() function of drm_encoder.
Cheers,
Biju
